fixes race condition in metadata consistency check#3392
Merged
keith-turner merged 2 commits intoapache:2.1from May 11, 2023
Merged
fixes race condition in metadata consistency check#3392keith-turner merged 2 commits intoapache:2.1from
keith-turner merged 2 commits intoapache:2.1from
Conversation
While looking into apache#3386 I noticed the Accumulo metadata consistency check was incrementing a counter in the incorrect place. It should increment before writing to the metadata table, but it does not. This could cause the check to report false postives. The false positive in the case would be transient and should not repeat on subsequent checks. Also noticed a redundant check when deciding if the file should be added to the set of in memory files. AFICT this redundant check is harmless, but it could cause problems for future changes.
Contributor
Author
|
Best to ignore whitespace when looking at the diffs |
dlmarion
reviewed
May 11, 2023
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java
Show resolved
Hide resolved
Contributor
Author
|
I am trying to trigger the false positive with the following test. I set the following properties. Then I run the following code in jshell client.tableOperations().create("foo");
var bw = client.createBatchWriter("foo");
SortedSet<Text> splits = new TreeSet<>();
IntStream.range(1, 100).mapToObj(i -> String.format("%03d", i)).map(Text::new)
.forEach(splits::add);
client.tableOperations().addSplits("foo", splits);
while (true) {
IntStream.range(1, 100).mapToObj(i -> String.format("%03d", i)).forEach(row -> {
Mutation mutation = new Mutation(row);
mutation.put("f1", "q1", "v1");
try {
bw.addMutation(mutation);
} catch (Exception e) {
throw new RuntimeException(e);
}
});
bw.flush();
client.tableOperations().flush("foo", null, null, true);
}I am seeing around 40 minor compactions per second while this test is running against 2.1.0. Not seeing it run into the race condition yet. I think I need to more tservers doing more checks against more tablets to increase the probability of bumping into it. I am running it under Uno. |
Contributor
Author
|
Looking at the code I noticed the increment for the bulk import code is also in the wrong place. Going to push a fix for that. |
dlmarion
approved these changes
May 11, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
While looking into #3386 I noticed the Accumulo metadata consistency check was incrementing a counter in the incorrect place. It should increment before writing to the metadata table, but it does not. This could cause the check to report false postives. The false positive in the case would be transient and should not repeat on subsequent checks.
Also noticed a redundant check when deciding if the file should be added to the set of in memory files. AFICT this redundant check is harmless, but it could cause problems for future changes.