[HUDI-3213] Making commit preserve metadata to true for compaction #4811

nsivabalan · 2022-02-14T11:37:27Z

What is the purpose of the pull request

Flipping the value of preserver commit metadata for compaction to true. Disabled for metadata table since meta fields are not populated.

To discuss:
when populate meta fields are disabled, should be check that preserve metadata can't be set to true. this might be an issue w/ kafka connect where we directly create log files.

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.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java

xushiyan · 2022-02-20T23:44:12Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java

        if (preserveMetadata) {
+          // do not preserve FILENAME_METADATA_FIELD


is it always the case where we never preserve FILENAME ? since when compact we always create new files, right? then it's some extra rule we have to remember for this config, which sounds like we preserve all meta cols.

i don't have a better idea. just felt the confusion here.

yes. so by preserve commit metadata, implicitly we mean to preserve every commit metadata(record key, partition path, commit time, commit seq no) except filename. Every other meta field should be carry forwarded from old record, but filename has to represent the file where the record actually exists.

...di-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java

nsivabalan · 2022-02-21T22:37:08Z

@xushiyan : this might need some work to be done. I will sync up w/ you directly. Fix is not trivial as I anticipated it to be.

hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java

nsivabalan · 2022-02-23T11:32:39Z

@xushiyan : this is good to review.

nsivabalan · 2022-02-28T15:32:18Z

@xushiyan : if you are occupied, I can ask sagar or ethan to review this patch. let me know.

codope

Please check if my below comment holds for this patch.

hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java

hudi-bot · 2022-03-07T05:54:56Z

CI report:

8ccef8c Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

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

xushiyan

@nsivabalan hey sorry i failed in catching up with notifications. Changes look okay. The comments below are optional or follow-up.

xushiyan · 2022-03-07T13:47:47Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java

-        IndexedRecord recordWithMetadataInSchema = rewriteRecord((GenericRecord) indexedRecord.get());
-        if (preserveMetadata) {
+        IndexedRecord recordWithMetadataInSchema = rewriteRecord((GenericRecord) indexedRecord.get(), preserveMetadata, oldRecord);
+        if (preserveMetadata && useWriterSchema) { // useWriteSchema will be true only incase of compaction.


if this is the case, better name could be useWriterSchemaForCompaction

xushiyan · 2022-03-07T13:49:03Z

hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java

    return newRecord;
  }

+  public static GenericRecord rewriteRecord(GenericRecord genericRecord, Schema newSchema, boolean copyOverMetaFields, GenericRecord fallbackRecord) {


would be good to have at least 1 UT covering this

xushiyan · 2022-03-07T13:51:13Z

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecord.java

  public static final String OPERATION_METADATA_FIELD = "_hoodie_operation";
  public static final String HOODIE_IS_DELETED = "_hoodie_is_deleted";

+  public static int FILENAME_METADATA_FIELD_POS = 4;


this could be a separate cleanup task: make constants for all meta fields and adopt them across codebase

…pache#4811) * Making commit preserve metadata to true * Fixing integ tests * Fixing preserve commit metadata for metadata table * fixed bootstrap tests * temp diff * Fixing merge handle * renaming fallback record * fixing build issue * Fixing test failures

nsivabalan changed the title ~~[HUDI-3213][WIP] Making commit preserve metadata to true~~ [HUDI-3213][WIP] Making commit preserve metadata to true for compaction Feb 14, 2022

nsivabalan mentioned this pull request Feb 14, 2022

[HUDI-3398] TableSchemaResolver may fail for metadata table #4782

Merged

5 tasks

nsivabalan force-pushed the compactPreserveCommitMetadata branch from 9ae9222 to 7ed0a4a Compare February 15, 2022 21:21

nsivabalan assigned xushiyan Feb 16, 2022

nsivabalan force-pushed the compactPreserveCommitMetadata branch from 480bf89 to 082ec6b Compare February 17, 2022 11:58

xushiyan reviewed Feb 20, 2022

View reviewed changes

xushiyan added priority:critical Production degraded; pipelines stalled writer-core labels Feb 20, 2022

xushiyan reviewed Feb 20, 2022

View reviewed changes

...di-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java Outdated Show resolved Hide resolved

nsivabalan force-pushed the compactPreserveCommitMetadata branch 2 times, most recently from f80680b to b8f3071 Compare February 21, 2022 20:14

nsivabalan force-pushed the compactPreserveCommitMetadata branch 2 times, most recently from 6ed7704 to ba8eca5 Compare February 22, 2022 23:35

nsivabalan changed the title ~~[HUDI-3213][WIP] Making commit preserve metadata to true for compaction~~ [HUDI-3213] Making commit preserve metadata to true for compaction Feb 23, 2022

nsivabalan commented Feb 23, 2022

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java Outdated Show resolved Hide resolved

nsivabalan mentioned this pull request Feb 25, 2022

[HUDI-3213] compaction should not change commit metadata #4619

Closed

nsivabalan force-pushed the compactPreserveCommitMetadata branch from ba8eca5 to 38cf843 Compare March 2, 2022 22:18

codope reviewed Mar 6, 2022

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java Outdated Show resolved Hide resolved

nsivabalan added 8 commits March 6, 2022 19:33

Making commit preserve metadata to true

75583d8

Fixing integ tests

f535954

Fixing preserve commit metadata for metadata table

16eddfc

fixed bootstrap tests

6a244bc

temp diff

8b55003

Fixing merge handle

0f991a1

renaming fallback record

8daae6a

fixing build issue

0d32395

Fixing test failures

8ccef8c

nsivabalan force-pushed the compactPreserveCommitMetadata branch from 1b14058 to 8ccef8c Compare March 7, 2022 03:43

codope approved these changes Mar 7, 2022

View reviewed changes

codope merged commit 3539578 into apache:master Mar 7, 2022

xushiyan reviewed Mar 7, 2022

View reviewed changes

		if (preserveMetadata) {
		// do not preserve FILENAME_METADATA_FIELD

[HUDI-3213] Making commit preserve metadata to true for compaction #4811

[HUDI-3213] Making commit preserve metadata to true for compaction #4811

Uh oh!

Conversation

nsivabalan commented Feb 14, 2022 • 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!

Uh oh!

Uh oh!

xushiyan Feb 20, 2022

Choose a reason for hiding this comment

Uh oh!

xushiyan Feb 20, 2022

Choose a reason for hiding this comment

Uh oh!

nsivabalan Feb 21, 2022 • 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!

Uh oh!

nsivabalan commented Feb 21, 2022

Uh oh!

Uh oh!

nsivabalan commented Feb 23, 2022

Uh oh!

nsivabalan commented Feb 28, 2022

Uh oh!

codope left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hudi-bot commented Mar 7, 2022

CI report:

Uh oh!

xushiyan left a comment

Choose a reason for hiding this comment

Uh oh!

xushiyan Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

xushiyan Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

xushiyan Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nsivabalan commented Feb 14, 2022 •

edited

Loading

nsivabalan Feb 21, 2022 •

edited

Loading