Move time source from db layer to PersistenceManager for better separation of concerns

[bowenxia]

Is your feature request related to a problem? Please describe.
Currently, the timeSrc field is being handled at the DB layer, but this implementation has raised some concerns about its correctness and responsibility allocation. Specifically, the timestamp should ideally be determined at the PersistenceManager level rather than the DB layer, which should solely focus on translating entity records into database queries without adding additional logic.

Problem Statement
The timeSrc field is currently being used within the DB layer to determine timestamps, including during query generation.
While this approach enables unit test validation by replacing time.Now() with a mockable db.timeSrc.Now(), it introduces a mixing of responsibilities.
The proper handling of timestamps should occur at the PersistenceManager level, ensuring better separation of concerns and adherence to the single responsibility principle.

Proposed Solution
Refactor the system so that the PersistenceManager level handles the timeSrc field.
The DB layer should only translate entity records into database queries without modifying or adding any additional information like timestamps.
This would involve removing the timestamp logic from the DB layer and ensuring it is fully managed by the upper layers.

Additional context
See discussion in this PR: #6593

[ribaraka]

Hello! I’d like to work on this task. I have a couple of questions so far.
Could you confirm that the PersistenceManager is simply an abstract concept referring to a higher-level service (or layer) that orchestrates persistence operations? I ask because the term is written together, which makes it a bit confusing 🥲 And I found that there is indeed a component called PersistenceManager in the iWF project, but I want to know that this term has no relation to the iWF. Apologies if this question seems trivial.

Regarding the logic for generating timestamps: Based on this discussion, the InsertReplicationTask method should not generate a new timestamp (i.e, created_time), instead, this responsibility lies with the PersistenceManager 🥲 . I also found that the required timestamp is already provided in the persistence.InternalReplicationTaskInfo struct via the CreationTime field. To confirm, in cases like this, I won’t regenerate the current time but will use the existing timestamp provided in the struct. Is this correct? Additionally, if this approach applies to similar cases, can I assume it’s best practice to avoid generating new timestamps and instead rely on existing ones where available?

Thank you for your guidance! 💪

[ribaraka]

New Development:
I proposed an approach earlier regarding handling the timeStamp in the InsertReplicationTask method. However, how should this be addressed in other methods? For instance, the InsertWorkflowExecutionWithTasks method has a similar parameter (replicationTasks []*nosqlplugin.ReplicationTask), which can be empty in some cases.
Relying on replicationTasks to extract a timeStamp in one method (e.g., InsertReplicationTask) while using a different field in another method (e.g., InsertWorkflowExecutionWithTasks) feels inconsistent and unstructured. If this acceptable, we can proceed with this.

Another approach I considered is introducing a new parameter to these methods for handling the current timeStamp. However, this would disrupt the existing interfaces, which isn’t ideal. If this approach is preferable I could go with this one, but probably I should think about maintaining backward compatibility with existing interfaces (input parameters for methods) if it is a concern.

A third option is to pass the current time directly into the Context. This might be a cleaner and more consistent solution, but I’m uncertain if it would misuse the concept of context. If it does, still, are you okay moving forward with this?

Sorry for asking so many questions upfront, I want to ensure I have a solid plan of how to proceed. If you have a better approach, please share with me.

[ribaraka]

^ @bowenxia, @taylanisikdemir, @3vilhamster, if you have a moment, could you please share your insights or advice that might help me with this dilemma?

[demirkayaender]

We are looking at this, and will respond with some pointers shortly.

[ribaraka]

@demirkayaender, I decided to take the initiative and move forward with the implementation I felt was suitable, which is - introducing a new parameter into the interface. I believe a visual example would make it clearer to determine the best approach, so I've submitted PR #6638 . I hope this helps draw attention and gather feedback on the current implementation—how it looks, what could be improved, and the next steps.

[bowenxia]

Hi @ribaraka, I am working on a thorough refactor doc. It will still need some time (1 day-ish) to finish and some time (1 day-ish) for our team to review. I'll get back to you once it is done hopefully by either tmr or early next week.

[bowenxia]

Hi @ribaraka , I've attached the refactor plan.

CDNC-12163.pdf

[ribaraka]

Hi @bowenxia, could you please review the current implementation of #6646? It follows your documentation (i.e. CDNC-12163.pdf). Please let me know how the code looks so far and whether I should continue in this manner.

[bowenxia]

Hi @ribaraka I've left a comment. Overall looks great :)

[bowenxia]

LGTM so far. Lemme know if you are planning to refactor other persistence managers here, or in a new PR.