-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[HUDI-7478] Fix max delta commits guard check w/ MDT #10820
[HUDI-7478] Fix max delta commits guard check w/ MDT #10820
Conversation
@hudi-bot run azure |
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.
aah. I was about to work on this fix just today. Can you write a quick UT for this please.
@@ -830,7 +830,7 @@ private static void deletePendingIndexingInstant(HoodieTableMetaClient metaClien | |||
protected static void checkNumDeltaCommits(HoodieTableMetaClient metaClient, int maxNumDeltaCommitsWhenPending) { | |||
final HoodieActiveTimeline activeTimeline = metaClient.reloadActiveTimeline(); | |||
Option<HoodieInstant> lastCompaction = activeTimeline.filterCompletedInstants() | |||
.filter(s -> s.getAction().equals(COMPACTION_ACTION)).lastInstant(); | |||
.filter(s -> s.getAction().equals(COMMIT_ACTION)).lastInstant(); |
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'm wondering whether we should use the COMPACTION_ACTION
for committed instant in release 1.0.x, cc @vinothchandar ~
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.
This check is valid under the assumption that the MDT table is definitely a MOR table.
800fcd3
to
c864618
Compare
@nsivabalan sorry for late reply. I've found existing test of MaxNumDeltacommitsWhenPending for COW table, so i've just parameterized it with HoodieTableType. Is it a quite quick and correct UT for this case? |
public void testMetadataTableWithLongLog() throws Exception { | ||
init(COPY_ON_WRITE, false); | ||
@ParameterizedTest | ||
@EnumSource(HoodieTableType.class) |
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.
can you write a new test for mor table specifically and to ensure the compaction is triggered and executed.
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.
don't think parametrizing catches the actual bug.
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.
hey @wombatu-kun : can you update your patch w/ below changes.
@Test
public void testMetadataTableWithLongLog() throws Exception {
init(COPY_ON_WRITE, false);
final int maxNumDeltacommits = 3;
writeConfig = getWriteConfigBuilder(true, true, false)
.withMetadataConfig(HoodieMetadataConfig.newBuilder()
.enable(true)
.enableMetrics(false)
.withMaxNumDeltaCommitsBeforeCompaction(5)
.withMaxNumDeltacommitsWhenPending(maxNumDeltacommits)
.build()).build();
initWriteConfigAndMetatableWriter(writeConfig, true);
// trigger 5 commits so that compaction gets triggered in MDT
for (int i = 1; i <= 5; i++) {
doWriteOperation(testTable, String.format("%016d", i));
}
// add a pending commit
testTable.addRequestedCommit(String.format("%016d", 6));
// add 3 more and we should hit exception.
for (int i = 7; i <= (7 + maxNumDeltacommits - 1); i++) {
doWriteOperation(testTable, String.format("%016d", i));
}
int instant = 7 + maxNumDeltacommits;
Throwable t = assertThrows(HoodieMetadataException.class, () -> doWriteOperation(testTable, String.format("%016d", instant)));
assertTrue(t.getMessage().startsWith(String.format("Metadata table's deltacommits exceeded %d: ", maxNumDeltacommits)));
}
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.
but it fails with:
AssertionFailedError: Expected org.apache.hudi.exception.HoodieMetadataException to be thrown, but nothing was thrown.
That is because all instants you create (String.format("%016d", i) == "000000000000i") are earlier than compaction instant, which is real timestamp like "20240314...".
I've added new test by myself, review please testMORCheckNumDeltaCommits
5899a0e
to
11a665f
Compare
.build(); | ||
initWriteConfigAndMetatableWriter(writeConfig, true); | ||
// write deltacommits to data-table and do compaction in metadata-table (with commit-instant) | ||
doWriteOperation(testTable, HoodieInstantTimeGenerator.formatDate(new Date())); |
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.
Maybe you can use InProcessTimeGenerator
which ensures the distinctness of each instant timestamp.
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.
thank you, i'll use it
// create pending instant in data table | ||
testTable.addRequestedCommit(HoodieInstantTimeGenerator.formatDate(new Date())); | ||
// continue writing | ||
while (activeTimeline.reload().getDeltaCommitTimeline().findInstantsAfter(lastCompaction.get().getTimestamp()).countInstants() <= maxNumDeltaCommits) { |
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.
Keep cautious the while loop may cause resource leaks. We better make this test commits deterministic.
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.
ok, fixed
2345fa1
to
425e59d
Compare
Change Logs
Here we account for action type "compaction". But compaction completed instant will have "commit" as action. So, we need to fix it.
Impact
low
Risk level (write none, low medium or high below)
low
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none".
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist