[HUDI-3919] [UBER] Support out of order rollback blocks in AbstractHoodieLogRecordReader #5341

suryaprasanna · 2022-04-18T06:17:37Z

What is the purpose of the pull request

This pull request adds support for out of order rollback blocks in AbstractHoodieLogRecordReader.

Brief change log

Change the iteration logic in AbstractHoodieLogRecordReader to support handling various multiwriter scenarios.
Fix issue when rollback blocks are farther from their target blocks
Include test case to verify out of order rollback blocks

Verify this pull request

This pull request change is already covered by existing tests, and also modified a new test case to verify the changes.

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

…odieLogRecordReader

nsivabalan · 2022-04-29T01:20:52Z

@alexeykudinkin : can you review this

prashantwason · 2022-05-10T22:08:33Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

@@ -218,7 +221,45 @@ protected synchronized void scanInternal(Option<KeySpec> keySpecOpt) {
          logFilePaths.stream().map(logFile -> new HoodieLogFile(new Path(logFile))).collect(Collectors.toList()),
          readerSchema, readBlocksLazily, reverseReader, bufferSize, enableRecordLookups, keyField, internalSchema);

+      /**
+       * Traversal of log blocks from log files can be done in two directions.


Please simplify this comment and provide only the current logic.

prashantwason · 2022-05-10T22:11:03Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

@@ -245,97 +286,53 @@ protected synchronized void scanInternal(Option<KeySpec> keySpecOpt) {
            continue;
          }
        }
+        if (logBlock.getBlockType().equals(CORRUPT_BLOCK)) {


Can we handle this too in the switch that follows? Having a common way to handle the various block types is easier to understand as per code flow.

prashantwason · 2022-05-10T22:11:40Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

+          continue;
+        }
+
+        // Rollback blocks contain information of instants that are failed, collect them in a set..


This comments seems more relevant to where the rollback block is being handled later.

prashantwason · 2022-05-10T22:14:13Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

        }
      }
+
+      int numBlocksRolledBack = 0;
+      // This is a reverse traversal on the collected data blocks.


collected data and delete blocks.

How is this reverse traversal? Isnt the for-loop a forward traversal?

prashantwason · 2022-05-10T22:15:25Z

hudi-common/src/test/java/org/apache/hudi/common/functional/TestHoodieLogFormat.java

@@ -839,20 +839,24 @@ public void testAvroLogRecordReaderWithRollbackTombstone(ExternalSpillableMap.Di
    writer.appendBlock(dataBlock);

    // Write 2
+    header = new HashMap<>();


header.clear() also works instead of allocating a new hashmap each time.

prashantwason · 2022-05-10T22:20:44Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

@@ -218,7 +221,45 @@ protected synchronized void scanInternal(Option<KeySpec> keySpecOpt) {
          logFilePaths.stream().map(logFile -> new HoodieLogFile(new Path(logFile))).collect(Collectors.toList()),
          readerSchema, readBlocksLazily, reverseReader, bufferSize, enableRecordLookups, keyField, internalSchema);


Lets also remove the readBlocksLazily argument as it now required to be always true.

prashantwason · 2022-05-10T22:21:05Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

@@ -218,7 +221,45 @@ protected synchronized void scanInternal(Option<KeySpec> keySpecOpt) {
          logFilePaths.stream().map(logFile -> new HoodieLogFile(new Path(logFile))).collect(Collectors.toList()),
          readerSchema, readBlocksLazily, reverseReader, bufferSize, enableRecordLookups, keyField, internalSchema);



Lets also remove the reverseReader as it is no longer supported.

suryaprasanna · 2022-05-11T20:16:10Z

@prashantwason Addressed the review comments. Removed readBlocksLazily and reverseReader flags from AbstractHoodieLogRecordReader and log file reader classes.
Although, I have not removed following configs COMPACTION_LAZY_BLOCK_READ_ENABLE, COMPACTION_REVERSE_LOG_READ_ENABLE from HoodieWriteConfig object.

hudi-bot · 2022-05-11T21:10:27Z

CI report:

e7b633c UNKNOWN
4e7a316 Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

...di-client-common/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackHelper.java

nsivabalan · 2022-05-10T22:50:01Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

+       *  This becomes more complicated if we have compacted blocks, which are data blocks created using log compaction.
+       *  TODO: Include support for log compacted blocks. https://issues.apache.org/jira/browse/HUDI-3580
+       *
+       *  To solve this do traversal twice.


I assume we will employ two traversals only when in need. i.e. when minor compactions are enabled. If not, can we avoid it and fallback to old behavior ?

Two traversals is needed to support the multiwriter scenarios where we can have rollback way away from the original block it is targeting. With minor compaction it becomes more tricky since we can have compacted blocks comprising of other compacted blocks. So, tackling the multiwriter scenarios with this PR first.

nsivabalan

hey Surya, thanks for the patch. Wondering, for single writer scenario, do we think we can retain old behavior. only for multi-writer and minor log compactions, we might have to take the new route.

nsivabalan · 2022-05-11T23:16:36Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

@@ -232,7 +251,7 @@ protected synchronized void scanInternal(Option<KeySpec> keySpecOpt) {
            && !HoodieTimeline.compareTimestamps(logBlock.getLogBlockHeader().get(INSTANT_TIME), HoodieTimeline.LESSER_THAN_OR_EQUALS, this.latestInstantTime
        )) {
          // hit a block with instant time greater than should be processed, stop processing further
-          break;
+          continue;


why continue. blocks greater than latest known instant time can be skipped altogether right?

Thanks for catching this. It is a mistake I am removing this.

nsivabalan · 2022-05-11T23:22:22Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/HoodieLogFileReader.java

    this.enableRecordLookups = enableRecordLookups;
    this.keyField = keyField;
    this.internalSchema = internalSchema == null ? InternalSchema.getEmptyInternalSchema() : internalSchema;
-    if (this.reverseReader) {


why we are removing the reverse reader ? can you help me understand

My understanding is that when iterating in reverse order there is an issue when we encounter corrupt block. We cannot jump across the corrupt block since we dont have the block size stored at the end for them. So, we end up ignoring all the blocks older than the corrupt block.
That is a reason for removing the reverseReader lookup, since it cannot be handled.
It becomes more complicated when introducing log compaction. There we need to move the compacted blocks to a different slot. So, it is not straight forward traversal. So, removing this logic to reduce the complexity involved.

Please let me know, what do you think?

nsivabalan · 2022-05-11T23:31:32Z

hudi-common/src/test/java/org/apache/hudi/common/functional/TestHoodieLogFormat.java

@@ -414,7 +411,7 @@ public void testHugeLogFileWrite() throws IOException, URISyntaxException, Inter
    header.put(HoodieLogBlock.HeaderMetadataType.SCHEMA, getSimpleSchema().toString());
    byte[] dataBlockContentBytes = getDataBlock(DEFAULT_DATA_BLOCK_TYPE, records, header).getContentBytes();
    HoodieLogBlock.HoodieLogBlockContentLocation logBlockContentLoc = new HoodieLogBlock.HoodieLogBlockContentLocation(new Configuration(), null, 0, dataBlockContentBytes.length, 0);
-    HoodieDataBlock reusableDataBlock = new HoodieAvroDataBlock(null, Option.ofNullable(dataBlockContentBytes), false,
+    HoodieDataBlock reusableDataBlock = new HoodieAvroDataBlock(null, Option.ofNullable(dataBlockContentBytes),


do we have tests for test out the multi-writer scenario. i.e rollback log block is appended after few other valid log blocks? if not, can we add one.

Sure, I will add them.

suryaprasanna · 2022-05-12T22:25:15Z

hey Surya, thanks for the patch. Wondering, for single writer scenario, do we think we can retain old behavior. only for multi-writer and minor log compactions, we might have to take the new route.

Does that mean we need to pass multiwriter enabled flag to AbstractHoodieLogRecordReader and using that flag toggle the logic between one traversal and two traversals?

nsivabalan · 2022-09-22T00:55:56Z

Closing in favor of #5958

suryaprasanna force-pushed the merged-log-block-dev branch 2 times, most recently from e7b633c to 8cdf174 Compare April 19, 2022 17:01

suryaprasanna changed the title ~~[HUDI-3580] [UBER] Add support for compacted log blocks~~ [HUDI-3919] [UBER] Support out of order rollback blocks in AbstractHoodieLogRecordReader Apr 19, 2022

[HUDI-3919] [UBER] Support out of order rollback blocks in AbstractHo…

5831902

…odieLogRecordReader

suryaprasanna force-pushed the merged-log-block-dev branch from 8cdf174 to 5831902 Compare April 19, 2022 20:22

nsivabalan assigned prashantwason Apr 29, 2022

nsivabalan self-assigned this May 3, 2022

prashantwason reviewed May 10, 2022

View reviewed changes

prashantwason requested changes May 10, 2022

View reviewed changes

Remove reverseReader and readBlocksLazily configs

4e7a316

nsivabalan reviewed May 11, 2022

View reviewed changes

vinothchandar assigned vinothchandar and unassigned nsivabalan Jul 13, 2022

xushiyan assigned prasannarajaperumal and xushiyan Aug 15, 2022

yihua added priority:critical production down; pipelines stalled; Need help asap. writer-core Issues relating to core transactions/write actions labels Sep 13, 2022

suryaprasanna closed this Sep 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-3919] [UBER] Support out of order rollback blocks in AbstractHoodieLogRecordReader #5341

[HUDI-3919] [UBER] Support out of order rollback blocks in AbstractHoodieLogRecordReader #5341

suryaprasanna commented Apr 18, 2022 •

edited

nsivabalan commented Apr 29, 2022

prashantwason May 10, 2022

prashantwason May 10, 2022

prashantwason May 10, 2022

prashantwason May 10, 2022

prashantwason May 10, 2022

prashantwason May 10, 2022

prashantwason May 10, 2022

suryaprasanna commented May 11, 2022

hudi-bot commented May 11, 2022

nsivabalan May 10, 2022

suryaprasanna May 12, 2022

nsivabalan left a comment

nsivabalan May 11, 2022

suryaprasanna May 12, 2022

nsivabalan May 11, 2022

suryaprasanna May 12, 2022

suryaprasanna May 12, 2022

nsivabalan May 11, 2022

suryaprasanna May 12, 2022

suryaprasanna commented May 12, 2022

nsivabalan commented Sep 22, 2022

		@@ -218,7 +221,45 @@ protected synchronized void scanInternal(Option<KeySpec> keySpecOpt) {
		logFilePaths.stream().map(logFile -> new HoodieLogFile(new Path(logFile))).collect(Collectors.toList()),
		readerSchema, readBlocksLazily, reverseReader, bufferSize, enableRecordLookups, keyField, internalSchema);

[HUDI-3919] [UBER] Support out of order rollback blocks in AbstractHoodieLogRecordReader #5341

[HUDI-3919] [UBER] Support out of order rollback blocks in AbstractHoodieLogRecordReader #5341

Conversation

suryaprasanna commented Apr 18, 2022 • edited

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

nsivabalan commented Apr 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suryaprasanna commented May 11, 2022

hudi-bot commented May 11, 2022

CI report:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsivabalan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suryaprasanna commented May 12, 2022

nsivabalan commented Sep 22, 2022

suryaprasanna commented Apr 18, 2022 •

edited