-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Always keep the compact revision, and delay compact tombstone revision #18162
Conversation
Always keep the compact revision, no matter it's a tomestone revision or not. If it's a tombstone revision, then cleanup it in next compact operation. Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files
... and 25 files with indirect coverage changes @@ Coverage Diff @@
## main #18162 +/- ##
==========================================
- Coverage 68.94% 68.81% -0.14%
==========================================
Files 416 416
Lines 35127 35121 -6
==========================================
- Hits 24219 24169 -50
- Misses 9518 9556 +38
- Partials 1390 1396 +6 Continue to review full report in Codecov by Sentry.
|
Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
These are all the test cases I can think of, please feel free to add more. Most likely some of them have already been covered by existing test cases. I also added the test cases in the doc (see option 2) Test casesunit test on keyIndexcase 1: compact a tombstone revision X, it shouldn't be compacted. Next compaction should cleanup the revision X for both cases below,
case 2: compact a normal revision X, it shouldn't be compacted. In next compaction,
e2e test on watchcase 1: compact tomestone revisions shouldn't impact watch case 2: compact normal revisions shouldn't impact watch case 3: a new watch starting on a compacted tombstone revision should receive the delete event e2e test on hash valuescase 1: All members should have the same hash value up to the compacted revision after compacting a tombstone revision
case 2: all member should have the same hash value up to the compacted revision after compacting a non-tombstone revision
case 3: All members should have the same hash value up to a non-compacted revision after compacting a tombstone revision
case 4: All members should have the same hash value up to a non-compacted revision after compacting a non-tombstone revision
robustness testSupport compaction, and verify the impact on hash values and watch. |
Can we start implementing those tests? The bigger problem then the bug existing is fact that we don't have those tests already. Even before we have a fix, we can just add the test to codify the current behavior. It should give us better visibility into impact of when we implement the fix. /cc @MadhavJivrajani @siyuanfoundation @fuweid @jmhbnz @henrybear327 @ah8ad3 |
@serathius: GitHub didn't allow me to request PR reviews from the following users: ah8ad3. Note that only etcd-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@@ -221,11 +221,6 @@ func (ki *keyIndex) compact(lg *zap.Logger, atRev int64, available map[Revision] | |||
if revIndex != -1 { | |||
g.revs = g.revs[revIndex:] | |||
} | |||
// remove any tombstone | |||
if len(g.revs) == 1 && genIdx != len(ki.generations)-1 { |
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.
Hi @ahrtr I'm still working on this.
I handle this in a different way. However, I still need more time to add more test cases for that.
diff --git a/server/storage/mvcc/key_index.go b/server/storage/mvcc/key_index.go
index e19068f9c..e8223380e 100644
--- a/server/storage/mvcc/key_index.go
+++ b/server/storage/mvcc/key_index.go
@@ -223,8 +223,10 @@ func (ki *keyIndex) compact(lg *zap.Logger, atRev int64, available map[Revision]
}
// remove any tombstone
if len(g.revs) == 1 && genIdx != len(ki.generations)-1 {
- delete(available, g.revs[0])
- genIdx++
+ if g.revs[0].Main < atRev {
+ delete(available, g.revs[0])
+ genIdx++
+ }
}
}
@@ -243,7 +245,9 @@ func (ki *keyIndex) keep(atRev int64, available map[Revision]struct{}) {
if !g.isEmpty() {
// remove any tombstone
if revIndex == len(g.revs)-1 && genIdx != len(ki.generations)-1 {
- delete(available, g.revs[revIndex])
+ if g.revs[revIndex].Main < atRev {
+ delete(available, g.revs[revIndex])
+ }
}
}
}
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.
@@ -223,8 +223,10 @@ func (ki *keyIndex) compact(lg *zap.Logger, atRev int64, available map[Revision] } // remove any tombstone if len(g.revs) == 1 && genIdx != len(ki.generations)-1 { - delete(available, g.revs[0]) - genIdx++ + if g.revs[0].Main < atRev { + delete(available, g.revs[0]) + genIdx++ + } } }
It doesn't make sense, because if g.revs[0].Main < atRev
will be always false
, so it has the same effect as my change, which is much simpler.
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 pointing it out. Yes, when there is only one record and it's not the last generation, we should always keep it.
// operation. We need to clean it up in this round of compact | ||
// operation. Refer to discussion in | ||
// https://github.com/etcd-io/etcd/issues/18089. | ||
if len(g.revs) > 1 { |
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 we don't need this if here. Just let it check the following condition. It's more easy to understand and reason
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.
It seems right. If len(g.revs)==1
is true
, it means that lastCompactionRev must be equal to the tombstone rev. In this round of compaction, the tombstone revision must be < compaction rev.
So we can revert this change.
@ahrtr: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
I would like to start working on those tests but i am a little bit lost of track if anybody started it or not is there any update? |
Always keep the compact revision, no matter it's a tomestone revision or not. If it's a tombstone revision, then cleanup it in next compact operation.
PoC of option 2 in https://docs.google.com/document/d/1APJE38DxENsRLLVapRz1CQ6UD4-uYaFutO12Y01VcNw/edit
cc @fuweid
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.