Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
auth/store: save consistentIndex to fix a data corruption bug #11652
auth/store: save consistentIndex to fix a data corruption bug #11652
Changes from all commits
06ad533
08a8b80
f14d2a0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think logically we want to pass ConsistentIndexGetter to NewAuthStore(), so that the auth store can get consistent index from etcdserver, and writes it to backend meta bucket at the end of applying a raft entry (such as adding a user, deleting a user, etc). Not sure if this means a lot of extra complexity in terms of implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. passing ConsistentIndexGetter to NewAuthStore will cause the saveIndex funtion to repeat.it looks a little ugly and violates the principle of function reuse. this is what we did in the first version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the second version, we define a public method SaveIndex in mvcc/store and define a interface ConsistentIndexSyncer which has a SaveIndex method that implemented by mvcc/store.SaveIndex. then, we want to pass ConsistentIndexSyncer interface to NewAuthStore().
What you see is the third version. After weighing the pros and cons, we choose the third version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for trying multiple implementations, and I understand maybe the current implementation makes the most sense coding wise. However here is my push back to the current implementation: auth store's correctness should not depend on passing the store pointer to mvcc. Auth store should be relatively independent of mvcc, although they share the same backend.
cc @mitake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, consistentIndex is a global concept. It has nothing to do with mvcc. It is better to separate it from mvcc/store and encapsulate it in a separate package. When mvcc, lessor, auth and other modules need to prevent repeated execution of commands, call consistentIndex package Save methods to store. What do you think of this solution? but I'm not sure how much complexity this approach will cause. which solution do you prefer? it is a cricital bug for us,so we take a simple and safe approach to fix it.
thanks. @jingyih @mitake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree with you! @tangcong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. For short term we can get this merged and backported. Could you add a TODO for encapsulating consistentindex into a separate package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. @jingyih