[HUDI-1552] Improve performance of key lookups from base file in Metadata Table.

[prashantwason]

What is the purpose of the pull request

Improves the performance of key lookups from Metadata Table.

In my scale testing with 150 partitions and 100K+ files on HDFS, the time to read the key was reduced (100ms avg -> 10ms) and the total data read from the HFile was reduced (85MB -> 3MB). The size of the base file was 3MB so this means that the in-memory HFile block caching was also working.

Brief change log

1. Cache the KeyScanner across lookups so that the HFile index does not have to be read for each lookup.
2. Enable block caching in KeyScanner.
3. Move the lock to a limited scope of the code to reduce lock contention.

Verify this pull request

This pull request is already covered by existing tests, such as (please describe tests).

mvn test -pl hudi-client/hudi-spark-client -Dtest=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.

[codecov-io]

Codecov Report

Merging #2494 (59b919a) into master (d8af24d) will increase coverage by 17.89%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master    #2494       +/-   ##
=============================================
+ Coverage     51.53%   69.43%   +17.89%     
+ Complexity     3491      363     -3128     
=============================================
  Files           462       53      -409     
  Lines         21881     1963    -19918     
  Branches       2327      235     -2092     
=============================================
- Hits          11277     1363     -9914     
+ Misses         9624      466     -9158     
+ Partials        980      134      -846     

Flag	Coverage Δ	Complexity Δ
hudicli	?	?
hudiclient	?	?
hudicommon	?	?
hudiflink	?	?
hudihadoopmr	?	?
hudisparkdatasource	?	?
hudisync	?	?
huditimelineservice	?	?
hudiutilities	69.43% <ø> (-0.06%)	0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ies/sources/helpers/DatePartitionPathSelector.java	54.68% <0.00%> (-1.57%)	13.00% <0.00%> (ø%)
...e/hudi/common/util/queue/BoundedInMemoryQueue.java
...udi/operator/partitioner/BucketAssignFunction.java
...pache/hudi/operator/KeyedWriteProcessOperator.java
...i/common/model/OverwriteWithLatestAvroPayload.java
...til/jvm/HotSpotMemoryLayoutSpecification64bit.java
...e/hudi/common/model/HoodieRollingStatMetadata.java
...he/hudi/exception/HoodieNotSupportedException.java
...udi/common/table/timeline/dto/ClusteringOpDTO.java
.../apache/hudi/common/model/ClusteringOperation.java
... and 394 more

[vinothchandar]

The size of the base file was 3MB so this means that the in-memory HFile block caching was also working.

Trying to understand this part. Was the workload, trying to fetch all the keys out of the HFile or just 1?

[nsivabalan]

do you think one line code warrants a separate method? not sure if we really need this. just curious.

[prashantwason]

removed.

[prashantwason]

The size of the base file was 3MB so this means that the in-memory HFile block caching was also working.

Trying to understand this part. Was the workload, trying to fetch all the keys out of the HFile or just 1?

The workload was a commit followed by a Clean operation with num_versions_retained=1 so it will clean all partitions. Hence, number of key lookups should be equal to number of partitions and all the keys should have been read from the HFile.

[prashantwason]

With enableReuse=false, the caching of readers needs special handling because:

1. Multiple threads may call into HoodieBackedTableMetadata.getRecordByKeyFromMetadata() to read their respective keys
2. If enableReuse=false, then each of these threads will try to close the readers after reading the key

Hence, we essentially have two codepaths:

1. enableReuse=false then readers cannot be cached
2. enableReuse=true then the readers can be cached.

I have updated the patch to handle both these cases by modifying the openFileSliceIfNeeded function (renamed to getReader) which returns either:

1. cached readers when enableReuse=true
2. newly opened readers when enableReuse=false

[vinothchandar]

Comments on going back to using the member variables to have the open readers always and have getReaders() just assign new ones, if they are null. This style is what books like clean code typically recommend.

Otherwise LGTM

[vinothchandar]

We are sort of creating two code paths again for reuse and non-reuse. Can we please go back to just always initializing the member variable here always?
then closeIfNeeded() can continue to work with members alone.

[vinothchandar]

I prefer the previous approach for readability. i.e have it just close the member variables. If you disagree, may be we can chat about why you think this is better for reading. I did face this issue when I originally read the code here

[nsivabalan]

I'm sure you have tried it. But not sure why can't we achieve this w/o two diff set of variables for both code paths? Can't we have just one set of reader variables. one which gets closed and reopened every time (if reuse is not enabled), or the same one getting re-used (if config is enabled)

[prashantwason]

The reason is that we do not lock the entire function getRecordByKeyFromMetadata(). So if we have one set of the reader variables, one of the thread may be reading a key while the other thread may be calling a close() on the readers.

The getRecordByKeyFromMetadata() function does the following:

1. Get the correct readers (open new readers if reuse=false)
2. Read the key from the baseFileReader (reads key from HFile)
3. Bytes to HoodieRecordPayload conversion (bytes read in above step from HFile)
4. Read the key from the logRecordScanner (in-memory lookup)
5. Merge the two payloads to get the final value
6. Close the readers (if reuse=false)

We should only lock during Step 2 as HFile KeyScanner is not thread-safe. Rest of the steps can be done by multiple threads in parallel for max performance.

[nsivabalan]

thnx for the detailed explanation Prasanth. will sync up w/ you offline. but still have some doubts/clarifications.
wrt statement 'one thread could be reading a key, while another could be closing the readers".
Let's talk about 2 scenarios.
a. reuse is not enabled. every thread is going to reinitialize the reader and close in the end and has a local copy of reader. And hence should not be an issue.
b. reuse is enabled. close() is invoked only when the HoodieBackedMetadataTable itself will be closed. So, I thought no thread will invoke close() when some other thread is reading.
May be I am missing something here. as I told, will sync up directly.

[nsivabalan]

please correct me if I am wrong. within this method, we don't care about reuse enabled or not. So, wondering if we need to remove this comment?

[nsivabalan]

Are these addressed? can you please respond.

[prashantwason]

Fixed the comment.

[nsivabalan]

this is confusing to have same var names for both local variables and instance variables. Can you please change it. I was reviewing code assuming openReaders sets instance vars only to realize after a long time that these are local vars.

[nsivabalan]

may be we could name instance vars as cachedBaseFileReader and cachedLogFileReader. wdyt?

[prashantwason]

There was a review comment earlier due to which I changed from cachedBaseFileReader.

What about using localBaseFileReader and logRecordScanner?

[vinothchandar]

if we synchronize access here, why synchronize at the lower levels?

[prashantwason]

The only function called from within this synchronized block of code is the openReaders which itself is not synchronized.

[vinothchandar]

can we please go back to the previous style of lazily opening/closing depending on the config and hiding it from the flow of reading and processing the values.

Happy to jump of a call if needed. But typically, reusing fewer variables and fewer branching the better. This does add signficant tax when reading the code. Thats why I changed them this way

[prashantwason]

I changed to this because the previous style was not clear enough. : |

[nsivabalan]

Looks like you haven't addressed or responded to some of my comments. Can approve once addressed. mostly minor ones.

[nsivabalan]

Are these addressed? can you please respond.

[nsivabalan]

Do you think this might simplify things.

Pair<HoodieFileReader, HoodieMetadataMergedLogRecordScanner> readers = null;
if( ! reuse)
   readers = openReaders();
else if(cached readers are null)
   readers = openReaders();
   // update cached/instance variables for readers
else
  readers = Pair.of(cached base file reader, cached log file reader) 

[vinothchandar]

I suggest, we just stick to the instance variables and initialize/close them lazily as needed. This is typically the OO pattern

[prashantwason]

@vinothchandar We cannot use the instance variables across multiple threads when resuse=False.

Consider the two cases separately:

---------- reuse=FALSE

1. Each thread should open and close their own copy of readers
2. We cannot use instance variables

--------- reuse=TRUE

1. Each thread should share the same readers
2. The first thread should open the readers and initialize the instance variables

[nsivabalan]

@vinothchandar : Can we sync up on this sometime and get a closure. Would like to have this in before our next release. May be at the end of next week's sync meeting, we can discuss on this.

[vinothchandar]

@prashantwason and I already synced on this. Will be catching up on reviews.

[prashantwason]

@vinothchandar and I discussed simplifying this PR. The following changes are to be implemented:

1. Remove the "reuse" configuration as it does not make sense for performance reasons.

   • When timeline server is used, reuse should be on
   • When timeline server is not used, each executor has its own instance of the Metadata Reader and reuse is implicit.

2. Simplify the above code to use the instance variables

3. Locking is not required because of the usage pattern in Add hoodie-hadoop-mr module with support for InputFormat #1. Locking will still be required in HFileReader because KeyScanner is not thread safe.

I am working on updating this PR.

[prashantwason]

@vinothchandar PTAL.

[vinothchandar]

Let me take a stab at this and push a commit on top, if you don't mind @prashantwason

[vinothchandar]

@prashantwason We can remove the reuse configuration - i.e no need to have this behavior be user controlled.

but ultimately, we still need to close everything out, where metadata table is opened from executors. I am going to just introduce a boolean variable within HoodieBackedTableMetadata

[prashantwason]

Looks good @vinothchandar