[WIP] [HUDI-625] Fixing performance issues around DiskBasedMap & kryo

[vinothchandar]

• This is very rough cut of few things I tried; Just for sharing purposes
• Kryo needs serializers and once we add them, the ser/deser is fast and writing finishes 10-20x faster
• DiskbasedMap is tracking too many things redundantly and incurring its cost as well.
• TODO : Need to break the kryo and map fix in differnt PRs
• TODO : For map entry thinning, need to handle compaction, fix code structure, tests
• TODO : For kyro, one more pass with good understanding of APIs, tests, null handling, cleanup

Tips

• Thank you very much for contributing to Apache Hudi.
• Please review https://hudi.apache.org/contributing.html before opening a pull request.

What is the purpose of the pull request

(For example: This pull request adds quick-start document.)

Brief change log

(for example:)

• Modify AnnotationLocation checkstyle rule in checkstyle.xml

Verify this pull request

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

(or)

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

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end.
• Added HoodieClientWriteTest to verify the change.
• Manually verified the change by running a job locally.

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.

[lamberken]

This is a great start. 👍

IMO, because we already set InstantiatorStrategy, so we needn't register class agian.
kryo.setInstantiatorStrategy(new org.objenesis.strategy.StdInstantiatorStrategy());

From kryo guide[1], we also modify this:

kryo.setInstantiatorStrategy(new Kryo.DefaultInstantiatorStrategy(new StdInstantiatorStrategy()));

So, we do bellow changes will be ok

• Change Kryo kryo = new KryoBase(); to Kryo kryo = new Kryo();
• kryo.setInstantiatorStrategy(new Kryo.DefaultInstantiatorStrategy(new StdInstantiatorStrategy()));

[1] https://github.com/EsotericSoftware/kryo#object-creation

[n3nash]

@vinothchandar this is interesting, why does Kryo need serializer implementations ? Ohh, just saw @lamber-ken's response -> the StdInstantiatorStrategy will allow kryo to fall back to Java Serde, is that what we want ?

[lamberken]

Hi @n3nash, we talked about this issue in HUDI-625 .

[vinothchandar]

@lamber-ken has a smaller fix that probably does not involve explicit serializers. but the way we implemented kryo Serialization is definitely problematic.

[vinothchandar]

@lamber-ken Please assign HUDI-625 to yourself, when aapche issues site comes back online.. I will take the DiskBasedMap related changes from here in a separae JIRA..

@n3nash wdyt ? I think the per-record overhead of HoodieRecord need not be persisted to the DiskBasedMap i.e just have <RecordKey, Payload>, others we can dynamically add when reading the map..

[lamberken]

Thanks @vinothchandar, had opend a new pr #1352 : )

[lamberken]

response -> the StdInstantiatorStrategy will allow kryo to fall back to Java Serde, is that what we want ?

Hi @n3nash, By default, an instantiator is returned that uses reflection if the class has a zero argument constructor, an exception is thrown. If a setInstantiatorStrategy(InstantiatorStrategy) is set, it will be used instead of throwing an exception.

More, the key point is the previous KryoBase in org.apache.hudi.common.util.SerializationUtils was used in a wrong way.

The "super.getInstantiatorStrategy().newInstantiatorOf(type).newInstance();", this will cause call it each time

[vinothchandar]

Will open a new PR with just diskmap changes.. HUDI-635 will track it

[vinothchandar]

These are no longer needed

[vinothchandar]

Clo