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

Get rid of OnPreCommitUnsafe and LockInsideApply & LockOutsideApply #16749

Open
ahrtr opened this issue Oct 12, 2023 · 1 comment
Open

Get rid of OnPreCommitUnsafe and LockInsideApply & LockOutsideApply #16749

ahrtr opened this issue Oct 12, 2023 · 1 comment
Assignees

Comments

@ahrtr
Copy link
Member

ahrtr commented Oct 12, 2023

What would you like to be added?

Previously the data inconsistent issue was introduced in #12964 and #12855 (added OnPreCommitUnsafe in #12855). In order to fix it, I introduced the ugly LockInsideApply & LockOutsideApply in #13854. It makes the consistent_index related logic is even more complicated and error prone. It needs huge caution each time when we update consistent_index related source code.

Since the first day when I started to fix #13766, I have been thinking to get rid of OnPreCommitUnsafe and LockInsideApply & LockOutsideApply, so as to improve readability and maintainability of the project.

Why is this needed?

Improve readability and maintainability

@ahrtr ahrtr self-assigned this Oct 12, 2023
@serathius
Copy link
Member

I have been looking into this too. My idea was to:

  • Remove direct access to backend outside of etcdserver. Instead provide open the transaction early and pass it down the call stack.
  • Remove the saving CI from backend hooks, instead make them a txn hook that is only provided in the apply loop.
  • To avoid performance degradation due to opening transaction early, have backend return a lazy transaction, that starts the real transaction only after first operation.

Benefits:

  • Makes bumping CI work as expected, only when executing a Write in apply loop.
  • Simplifies
  • Each operation gets the minimal txn interface. Either Write or Read, but not whole backend.

Downsides:

  • A lot of functions will get the txn argument.

Problems:

  • Txn operation does a weird txn hack. It first opens a read transaction to do Txn validation and checking conditions, and then if needed it closes the read transaction and opens a write transaction. This works only because it's executed in apply loop and only the apply loop writes.

Please let me know if that makes sense, or you are thinking about other approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants