HBASE-21426 TestEncryptionKeyRotation.testCFKeyRotation is flaky #375
Conversation
This file contains 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
sunhelly
changed the title
xcangCRM
reviewed
Jul 12, 2019
| return !found; | ||
| public boolean evaluate() throws IOException { | ||
| return TEST_UTIL.getAdmin().getCompactionState(htd.getTableName()) == | ||
| CompactionState.NONE; |
There was a problem hiding this comment.
So here you are checking table's compaction state is "NONE" to determine compaction is done.
How is this sufficient? Can this state never changed? Thanks
There was a problem hiding this comment.
Hi, @xcangCRM , thanks your questions.
How is this sufficient? Can this state never changed?
We all know majorCompact() is a async method, returning not means compaction completed.
But the method will return after building the compaction context and adding the compaction to the executor pool. And before the executor runs compaction, when building the compaction context, state of table regions that want to compact will be set to MAJOR by calling reportCompactionRequestStart(). So calling majorCompact() returns means table compaction state is not 'NONE' until the major compaction is completed.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
infraio
reviewed
Jul 16, 2019
| @@ -91,6 +92,7 @@ public static void setUp() throws Exception { | |||
| conf.setInt("hfile.format.version", 3); | |||
| conf.set(HConstants.CRYPTO_KEYPROVIDER_CONF_KEY, KeyProviderForTesting.class.getName()); | |||
| conf.set(HConstants.CRYPTO_MASTERKEY_NAME_CONF_KEY, "hbase"); | |||
| conf.setInt("hbase.hfile.compaction.discharger.interval", 10 * 60 * 1000); | |||
There was a problem hiding this comment.
@infraio Thanks for reviewing.
Why this config change?
I changed this to let the compacted files be cleaned up slower than normal(10min instead of 2min). Because when I run testCFKeyRotation() for 100 times, I have met assertion fail at List<Path> compactedPaths = findCompactedStorefilePaths(htd.getTableName()); assertTrue(compactedPaths.size() > 0);. So this test case may also be flaky at this assertion.
Enlarging the compaction discharger interval can ensure compacted files still be there after major compaction is completed but compacted paths are not checked in testCFKeyRotation().
There was a problem hiding this comment.
How about a bigger value? The test will timeout if it can't finished after 780 seconds. So may be use 30 * 60 * 1000 is better which is bigger than 2 * 780?
There was a problem hiding this comment.
I checked the test class config, it's a MediumTests.class, which will complete in 50 seconds. So the test method testCFKeyRotation() must complete in 50 secs, less than the cleaner interval(2 mins). I met this problem because when I ran 100 times of the method, I used parameterized running. All 100 cases used the same minicluster and when ran after 2 mins the case may met with checking compacted files fail.
@infraio I will convert the change of the conf.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
infraio
approved these changes
Jul 18, 2019
asfgit
pushed a commit
that referenced
this pull request
Jul 19, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit
pushed a commit
that referenced
this pull request
Jul 19, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
infraio
pushed a commit
to infraio/hbase
that referenced
this pull request
Aug 17, 2020
…che#375) Signed-off-by: Guanghao Zhang <zghao@apache.org>
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.
No description provided.