Moving AckManager to DB: - Githubissues

temporalio / temporal

Temporal service

https://docs.temporal.io

MIT License

10.3k stars 754 forks source link

Moving AckManager to DB: #5922

Closed Shivs11 closed 2 weeks ago

Shivs11 commented 2 weeks ago

What changed?

This PR, initially, aimed to extract AckLevel inside of db from the AckManager. This is done so that we persist AckLevels with every batch of tasks that are created.
However, if we do only extract AckLevel into db, we would still be querying AckManager for readLevel (amongst other variables) which would make life harder and code unclearer in the near future.
Thus, I propose refactoring by merging AckManager inside of db altogether. This shall also be in agreement with our long term plan of merging code entirely into backlogManager in the near future.

Why?

Cleaner code
Required functionality for accurate workings of the backlog counter

How did you test it?

Existing suite of tests (CI)

Potential risks

None, merging into my own feature branch

Is hotfix candidate?

No

Shivs11 commented 2 weeks ago

This PR is going to be marked as closed. Discussions with @ShahabT proved that merging AckManager into db, as was done here, was wrong since write latencies could occur. Moreover, moving ackLevel into db also did not make sense as db would then in-turn have to wait for AckManager's lock which could further cause latencies.

Thus, persisting the latest AckLevel with task creation, as a result, will also not be done at this moment in time.