[HUDI-1294] Adding inline read and seek based read(batch get) for hfile log blocks in metadata table #3762

nsivabalan · 2021-10-08T01:34:26Z

What is the purpose of the pull request

Added support to read HFile log blocks via inline FileSystem in metadata table.
Also added support to read for a list of keys(batch get) rather than full scan in metadata table.

Brief change log

Added two new configs to HoodieMetadataConfig. hoodie.metadata.enable.inline.reading.log.files and hoodie.metadata.enable.full.scan.log.files.
Since we are adding support for seek based read, renamed AbstractHoodieLogRecordScanner to AbstractHoodieLogRecordReader. and so have renamed HoodieMetadataMergedLogRecordReader.
Added new method to HoodieMetadataMergedLogRecordReader to support this purpose(i.e. reading records for a list of keys) w/o doing full scan.

public List<Pair<String, Option<HoodieRecord<HoodieMetadataPayload>>>> getRecordsByKeys(List<String> keys) {

}

Added new method to HoodieDataBlock for the new requirement. Base class does not have any impl. HoodieHFileDataBlock overrides and gives a concrete impl where in records are read via inline FileSystem with seek based approach.

public List<IndexedRecord> getRecords(List<String> keys) throws IOException {
}

HoodieDataBlock also adheres to enableInline config even if not for batch get. Basically 3 options are possible. a: full scan w/o inline. b. full scan with inlining. c. batch get (with inline)
Have fixed BaseTableMetadata.getAllFilesInPartitions(List partitions) to use batch get api rather than N single calls to getAllFilesInPartition(Path partitionPath).
have fixed metadata reader (HoodieBackedTableMetadata) to leverage the new apis based on config values.

Verify this pull request

This change added tests and can be verified as follows:

Added tests to TestHoodieRealtimeRecordReader to verify the change.
Found some gaps in testing HFileWriter and Reader especially around seek based read and have added TestHoodieHFileReaderWriter to test these cases.
Enabled inline and batch get reads to 1 test in TestHoodieBackedMetadata.

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.

hudi-bot · 2021-10-08T01:38:47Z

CI report:

5fb7a2afa196fd75ada005d26a0fb9fce5472545 UNKNOWN
2b369a6 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

nsivabalan · 2021-10-08T01:41:57Z

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

+  }
+
+  public void scan(List<String> keys) {
+    currentInstantLogBlocks = new ArrayDeque<>();


One thing to be cautious about seek based approach vs full scan. In full scan, we do one time full scan and prepare a hashmap of records. so, any number of look up can be done without any cost.
But with seek based approach, if users calls
scan(list of 3 keys)
scan(list of 5 keys)
we might have to read/parse through the log blocks twice, since everytime we are looking for only interested keys. so, we should be cautious in using the seek based read for metadata table.

This seems like a good point to add as a code comment in this file.

nsivabalan · 2021-10-13T06:05:56Z

@vinothchandar : this is good to be reviewed.

nsivabalan · 2021-10-13T16:29:21Z

hudi-common/src/main/java/org/apache/hudi/metadata/BaseTableMetadata.java

@prashantwason @satishkotha : do you guys know why we did not do batch get here and doing 1 key at a time? is there any particular reason for it. I have fixed it to fetch batch get in this patch.

For simplicity of implementation I suppose - performance was not taken into consideration. Also, given the number of keys being fetched, batch would be slower as it may need to read the entire hfile.

@umehrot2 Thoughts?

from what I infer, with HoodieMergedLogRecordScanner, we first read all records from all log blocks and prepare a hash map of records(record key to HoodieRecord). And we don't do seek based read prior to this patch and so we do read all log records from all log blocks. so was bit curious.

nsivabalan · 2021-10-13T23:23:59Z

@hudi-bot azure run

nsivabalan · 2021-10-14T02:01:44Z

@hudi-bot azure run

nsivabalan · 2021-10-14T06:51:28Z

@hudi-bot azure run

nsivabalan · 2021-10-14T06:54:43Z

@hudi-bot azure run

vinothchandar

Left some comments.

...hudi-client-common/src/test/java/org/apache/hudi/io/storage/TestHoodieHFileReaderWriter.java

hudi-common/src/main/java/org/apache/hudi/common/config/HoodieMetadataConfig.java

vinothchandar · 2021-10-20T00:48:25Z

hudi-common/src/main/java/org/apache/hudi/common/config/HoodieMetadataConfig.java

+      .key(METADATA_PREFIX + ".enable.full.scan.log.files")
+      .defaultValue(true)
+      .sinceVersion("0.10.0")
+      .withDocumentation("Enable full scanning of log files while reading log records");


little bit more context?

wrt your suggestion of moving this out to common config(instead of metadata config), I don't really see a need where we will use this for regular data table. so, I prefer we can leave it at metadata config itself. let me know.

hudi-common/src/main/java/org/apache/hudi/common/config/HoodieMetadataConfig.java

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

hudi-common/src/main/java/org/apache/hudi/metadata/BaseTableMetadata.java

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java

hudi-common/src/main/java/org/apache/hudi/metadata/BaseTableMetadata.java

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java

nsivabalan · 2021-10-20T20:23:57Z

@hudi-bot azure.

prashantwason · 2021-10-21T07:58:12Z

hudi-common/src/main/java/org/apache/hudi/common/config/HoodieMetadataConfig.java

interesting entries

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java

nsivabalan · 2021-10-25T05:35:38Z

@prashantwason : Can you review the patch please when you get time.

nsivabalan · 2021-10-28T14:40:49Z

@hudi-bot azure run

nsivabalan force-pushed the inlineMetadataLogReader branch from 5fb7a2a to cb7e9ce Compare October 8, 2021 01:38

nsivabalan changed the title ~~[HUDI-1294] Adding inline read and seekable read for hfile blocks in metadata table~~ [HUDI-1294] Adding inline read and seekable read for hfile log blocks in metadata table Oct 8, 2021

nsivabalan commented Oct 8, 2021

View reviewed changes

nsivabalan requested a review from vinothchandar October 8, 2021 01:42

nsivabalan changed the title ~~[HUDI-1294] Adding inline read and seekable read for hfile log blocks in metadata table~~ [HUDI-1294] Adding inline read and seek based read(batch get) for hfile log blocks in metadata table Oct 8, 2021

This was referenced Oct 11, 2021

[HUDI-1294][WIP] Adding inline read for Hfile log blocks #3752

Closed

[HUDI-431] [WIP] Adding parquet data block with inline read support #3783

Closed

nsivabalan force-pushed the inlineMetadataLogReader branch from 1140119 to f27df7a Compare October 13, 2021 05:59

nsivabalan assigned vinothchandar Oct 13, 2021

nsivabalan commented Oct 13, 2021

View reviewed changes

nsivabalan force-pushed the inlineMetadataLogReader branch 2 times, most recently from 8756437 to ce6740e Compare October 14, 2021 06:54

vinothchandar reviewed Oct 20, 2021

View reviewed changes

nsivabalan force-pushed the inlineMetadataLogReader branch from 48ed467 to f378067 Compare October 21, 2021 05:13

prashantwason reviewed Oct 21, 2021

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/config/HoodieMetadataConfig.java Outdated

Copy link

Member

prashantwason Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting entries

prashantwason reviewed Oct 21, 2021

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java Outdated Show resolved Hide resolved

nsivabalan added the priority:blocker Production down; release blocker label Oct 22, 2021

nsivabalan added 3 commits October 29, 2021 06:39

Adding inline read and seekable read for hfile blocks in metadata table

a444337

Fixing batch get files for list of partition in BaseTableMetadata

6084dbb

Addressing feedback

2b369a6

nsivabalan force-pushed the inlineMetadataLogReader branch from f378067 to 2b369a6 Compare October 29, 2021 13:40

nsivabalan merged commit 69ee790 into apache:master Oct 29, 2021

[HUDI-1294] Adding inline read and seek based read(batch get) for hfile log blocks in metadata table #3762

[HUDI-1294] Adding inline read and seek based read(batch get) for hfile log blocks in metadata table #3762

Uh oh!

Conversation

nsivabalan commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

hudi-bot commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

nsivabalan Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prashantwason Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsivabalan commented Oct 13, 2021

Uh oh!

nsivabalan Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

prashantwason Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

nsivabalan Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

nsivabalan commented Oct 13, 2021

Uh oh!

nsivabalan commented Oct 14, 2021

Uh oh!

nsivabalan commented Oct 14, 2021

Uh oh!

nsivabalan commented Oct 14, 2021

Uh oh!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vinothchandar Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

nsivabalan Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nsivabalan commented Oct 20, 2021

Uh oh!

prashantwason Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nsivabalan commented Oct 25, 2021

Uh oh!

nsivabalan commented Oct 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nsivabalan commented Oct 8, 2021 •

edited

Loading

hudi-bot commented Oct 8, 2021 •

edited

Loading

nsivabalan Oct 8, 2021 •

edited

Loading

prashantwason Oct 21, 2021 •

edited

Loading