Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[POC]: Replace OptimisticTransaction with WriteBatch #12478

Closed
Tracked by #12033
Zelldon opened this issue Apr 19, 2023 · 3 comments
Closed
Tracked by #12033

[POC]: Replace OptimisticTransaction with WriteBatch #12478

Zelldon opened this issue Apr 19, 2023 · 3 comments
Labels
area/performance Marks an issue as performance related component/db kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc.

Comments

@Zelldon
Copy link
Member

Zelldon commented Apr 19, 2023

Description

We have introduced the usage of OptimisticTransactionDB a while ago, because we want to be able to achieve atomicity within the processing of a command. Furthermore, that we can easily roll back our changes if a processing error appears.

Our thought was that we can easily archive this with the Transaction the OptimisticTransactionDB provides. One caveat is that we have always one writer, which means there will be never a case of a conflict, meaning conflict checking is not necessary.

Since we don't need conflict checking I'm questioning (@npepinpe already raised that as well before) whether we really need the OptimisticTransactionDB. As we can see in this comment, the conflict check involves checking timestamp of changed keys, etc which might be a expensive operation.

The transaction is internally based on a WriteBatch with on top having the conflict checking if we don't need the checks we can just base on WriteBatches, which can be written atomically as well. Rollback can easily archived via throwing the batch away, which means there will be not change on the database itself.

Accessing the data in the batch and database (Read-Your-Own-Writes) can be archived via WriteBatchWithIndex

I think it is worth to investigate whether it makes a difference to migrate to WriteBatches, and not using the Transactions anymore internally (the API of ZeebeDB will not change only the internal handling).

I would like to do a POC of this and to verify whether we get any performance gain out of it, which might also help in #12033

@Zelldon Zelldon added kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. area/performance Marks an issue as performance related component/db labels Apr 19, 2023
@megglos
Copy link
Contributor

megglos commented Apr 26, 2023

ZDP-Triage:

  • assuming this doesn't need triaging and will be handled by the linked epic DRI

@Zelldon
Copy link
Member Author

Zelldon commented May 3, 2023

Note: we have a second writer, which is the ExporterDirector, we need to verify whether this is an issue.

Related comment regarding writebatch that it might make sense if we don't need conflict checks https://groups.google.com/g/rocksdb/c/psEhYNnFnB8/m/z8trUJAfAQAJ?utm_medium=email&utm_source=footer

@Zelldon
Copy link
Member Author

Zelldon commented May 5, 2023

Based on the insights we have won here #12033 (comment) my current assumption is that this will not bring us much, so I will postpone it for now and close it.

@Zelldon Zelldon closed this as not planned Won't fix, can't repro, duplicate, stale May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance Marks an issue as performance related component/db kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc.
Projects
None yet
Development

No branches or pull requests

2 participants