HIVE-29420: Extra test coverage for Cleaner with minOpenWriteId flag#6286
HIVE-29420: Extra test coverage for Cleaner with minOpenWriteId flag#6286deniskuzZ merged 1 commit intoapache:masterfrom
Conversation
deniskuzZ
force-pushed
the
cleaner-test
branch
from
f2adf9c to
1f6fb58
Compare
deniskuzZ
force-pushed
the
cleaner-test
branch
from
1f6fb58 to
a65cc64
Compare
asf-ci-hive
added
tests pending
tests failed
tests passed
and removed
tests failed
tests pending
tests passed
labels
deniskuzZ
force-pushed
the
cleaner-test
branch
from
e0c4db8 to
104ed32
Compare
deniskuzZ
force-pushed
the
cleaner-test
branch
2 times, most recently
from
57409f2 to
34ec13f
Compare
deniskuzZ
force-pushed
the
cleaner-test
branch
2 times, most recently
from
ed654b1 to
8968220
Compare
asf-ci-hive
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
deniskuzZ
force-pushed
the
cleaner-test
branch
from
8968220 to
fb1d440
Compare
deniskuzZ
changed the title
deniskuzZ
force-pushed
the
cleaner-test
branch
from
fb1d440 to
a4e231b
Compare
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
| long compactorTxnId = compactInTxn(rqst, CommitAction.COMMIT); | ||
|
|
||
| // Wait for the cooldown period so the Cleaner can see the last committed txn as the highest committed watermark | ||
| // TODO: doesn't belong here, should probably be moved to CompactorTest#startCleaner() |
There was a problem hiding this comment.
Hi, I wonder, how many effort required to actually move that code to CompactorTest#startCleaner()?
There was a problem hiding this comment.
i thought about that and then noticed that startCleaner is used in plenty of tests, so adding 1s sleep there probably not the best idea, however, that could have been a cleaner change
i can try moving there and see how long the build takes after the change
There was a problem hiding this comment.
refactored by adding @SleepBeforeCleaner annotation
There was a problem hiding this comment.
dropped the refactor commit since many other tests failed as they didn't invoke startCleaner, but instead initiated cleaner directly.
it's a complete mess and out-of-scope of this PR
| // Wait for the cooldown period so the Cleaner can see the last committed txn as the highest committed watermark | ||
| // TODO: doesn't belong here, should probably be moved to CompactorTest#startCleaner() | ||
| Thread.sleep(MetastoreConf.getTimeVar( | ||
| conf, ConfVars.TXN_OPENTXN_TIMEOUT, TimeUnit.MILLISECONDS)); |
There was a problem hiding this comment.
long compactInTxn(CompactionRequest rqst, CommitAction commitAction) already contains this sleep. What is the reason of the extra wait?
There was a problem hiding this comment.
Never mind. I just saw the next change.
|
|
||
| CompactionRequest rqst = new CompactionRequest("default", "camtc", CompactionType.MAJOR); | ||
| long compactTxn = compactInTxn(rqst, CommitAction.MARK_COMPACTED); | ||
| addBaseFile(t, null, 25L, 25, 26); |
There was a problem hiding this comment.
Could you please explain why this addBaseFile moved here?
There was a problem hiding this comment.
i moved the compacted dir creation closer to the actual compaction txn that is supposed to create it
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
deniskuzZ
force-pushed
the
cleaner-test
branch
from
b958153 to
a4e231b
Compare
|
|
Thanks @InvisibleProgrammer, @kasakrisz for the reviews! |
What changes were proposed in this pull request?
Added new test
Why are the changes needed?
Improve test coverage
Does this PR introduce any user-facing change?
No
How was this patch tested?
Test