codope commented on code in PR #11137:
URL: https://github.com/apache/hudi/pull/11137#discussion_r1590529782
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -463,11 +460,11 @@ private String generateUniqueCommitInstantTime(String
initializationTime) {
if (HoodieTableMetadataUtil.isIndexingCommit(dataIndexTimeline,
initializationTime)) {
return initializationTime;
}
- // Add suffix to initializationTime to find an unused instant time for the
next index initialization.
+ // otherwise yields the timestamp on the fly.
// This function would be called multiple times in a single application if
multiple indexes are being
// initialized one after the other.
for (int offset = 0; ; ++offset) {
- final String commitInstantTime =
HoodieTableMetadataUtil.createIndexInitTimestamp(initializationTime, offset);
+ final String commitInstantTime =
HoodieInstantTimeGenerator.instantTimePlusMillis(SOLO_COMMIT_TIMESTAMP, offset);
Review Comment:
Why do we need the SOLO_COMMIT_TIMESTAMP again here? I think the
initializationTime passed to this method should already be
SOLO_COMMIT_TIMESTAMP if the timeline had no completed instants right? We do
this in
https://github.com/apache/hudi/blob/d4f55f193cd79f317d4fc021cc741554ce8cf6cd/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java#L250
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -463,11 +460,11 @@ private String generateUniqueCommitInstantTime(String
initializationTime) {
if (HoodieTableMetadataUtil.isIndexingCommit(dataIndexTimeline,
initializationTime)) {
return initializationTime;
}
- // Add suffix to initializationTime to find an unused instant time for the
next index initialization.
+ // otherwise yields the timestamp on the fly.
Review Comment:
let's move this comment to line 459, just before if blok.
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieMetadataBootstrap.java:
##########
@@ -167,20 +167,19 @@ public void testMetadataBootstrapInflightCommit() throws
Exception {
HoodieTableType tableType = COPY_ON_WRITE;
init(tableType, false);
+ // In real production env, bootstrap action can only happen on empty table,
+ // otherwise we need to roll back the previous bootstrap first,
+ // see 'SparkBootstrapCommitActionExecutor.execute' for more details.
doPreBootstrapWriteOperation(testTable, INSERT, "0000001");
doPreBootstrapWriteOperation(testTable, "0000002");
// add an inflight commit
HoodieCommitMetadata inflightCommitMeta =
testTable.doWriteOperation("00000007", UPSERT, emptyList(),
- asList("p1", "p2"), 2, true, true);
+ asList("p1", "p2"), 2, false, true);
Review Comment:
setting to `false` won't do any bootstrap. Can you please explain why
bootstrap is set to false?
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java:
##########
@@ -925,6 +925,37 @@ public void
testMetadataTableCompactionWithPendingInstants() throws Exception {
assertEquals(HoodieInstantTimeGenerator.instantTimeMinusMillis(inflightInstant2,
1L), tableMetadata.getLatestCompactionTime().get());
}
+ @Test
+ public void testInitializeMetadataTableWithPendingInstant() throws Exception
{
+ init(COPY_ON_WRITE, false);
+ initWriteConfigAndMetatableWriter(writeConfig, false);
+ doWriteOperation(testTable, metaClient.createNewInstantTime(), INSERT);
+ doWriteOperation(testTable, metaClient.createNewInstantTime(), INSERT);
+
+ // test multi-writer scenario. let's add 1,2,3,4 where 1,2,4 succeeded,
but 3 is still inflight. so latest delta commit in MDT is 4, while 3 is still
pending
+ // in DT and not seen by MDT yet. compaction should not trigger until 3
goes to completion.
Review Comment:
Test doesn't seem to do as commented.
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java:
##########
@@ -925,6 +925,37 @@ public void
testMetadataTableCompactionWithPendingInstants() throws Exception {
assertEquals(HoodieInstantTimeGenerator.instantTimeMinusMillis(inflightInstant2,
1L), tableMetadata.getLatestCompactionTime().get());
}
+ @Test
+ public void testInitializeMetadataTableWithPendingInstant() throws Exception
{
+ init(COPY_ON_WRITE, false);
Review Comment:
If we are going to for only one table type, shall we test for MOR instead of
COW? If the initialization works for MOR, it should work for COW as well.
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieMetadataBootstrap.java:
##########
@@ -261,12 +260,8 @@ private void bootstrapAndVerifyFailure() throws Exception {
writeConfig = getWriteConfig(true, true);
initWriteConfigAndMetatableWriter(writeConfig, true);
syncTableMetadata(writeConfig);
- try {
- validateMetadata(testTable);
- Assertions.fail("Should have failed");
- } catch (IllegalStateException e) {
- // expected
- }
+ Assertions.assertThrows(AssertionFailedError.class, () ->
validateMetadata(testTable),
Review Comment:
Should there be an assert or validation in the non-test code itself? I think
asserting `AssertionFailedError` is like swallowing any exception. We should
assert some specific exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
