Skip to content

Conversation

@vinothchandar
Copy link
Member

  • Introduced an internal metadata table, that stores file listings.
  • metadata table is kept upto date with
  • Fixed handling of CleanerPlan.
  • [HUDI-842] Reduce parallelism to speed up the test.
  • [HUDI-842] Implementation of CLI commands for metadata operations and lookups.
  • [HUDI-842] Extend rollback metadata to include the files which have been appended to.
  • [HUDI-842] Support for rollbacks in MOR Table.
  • MarkerBasedRollbackStrategy needs to correctly provide the list of files for which rollback blocks were appended.
  • [HUDI-842] Added unit test for rollback of partial commits (inflight but not completed yet).
  • [HUDI-842] Handled the error case where metadata update succeeds but dataset commit fails.
  • [HUDI-842] Schema evolution strategy for Metadata Table. Each type of metadata saved (FilesystemMetadata, ColumnIndexMetadata, etc.) will be a separate field with default null. The type of the record will identify the valid field. This way, we can grow the schema when new type of information is saved within in which still keeping it backward compatible.
  • [HUDI-842] Fix non-partitioned case and speedup initial creation of metadata table.Choose only 1 partition for jsc as the number of records is low (hundreds to thousands). There is more overhead of creating large number of partitions for JavaRDD and it slows down operations like WorkloadProfile.
    For the non-partitioned case, use "." as the name of the partition to prevent empty keys in HFile.
  • [HUDI-842] Reworked metrics pusblishing.
  • Code has been split into reader and writer side. HoodieMetadata code to be accessed by using HoodieTable.metadata() to get instance of metdata for the table.
    Code is serializable to allow executors to use the functionality.
  • [RFC-15] Add metrics to track the time for each file system call.
  • [RFC-15] Added a distributed metrics registry for spark which can be used to collect metrics from executors. This helps create a stats dashboard which shows the metadata table improvements in real-time for production tables.
  • [HUDI-1321] Created HoodieMetadataConfig to specify configuration for the metadata table. This is safer than full-fledged properties for the metadata table (like HoodieWriteConfig) as it makes burdensome to tune the metadata. With limited configuration, we can control the performance of the metadata table closely.

[HUDI-1319][RFC-15] Adding interfaces for HoodieMetadata, HoodieMetadataWriter (#2266)

  • moved MetadataReader to HoodieBackedTableMetadata, under the HoodieTableMetadata interface
  • moved MetadataWriter to HoodieBackedTableMetadataWriter, under the HoodieTableMetadataWriter
  • Pulled all the metrics into HoodieMetadataMetrics
  • Writer now wraps the metadata, instead of extending it
  • New enum for MetadataPartitionType
  • Streamlined code flow inside HoodieBackedTableMetadataWriter w.r.t initializing metadata state
  • [HUDI-1319] Make async operations work with metadata table ([HUDI-1319] Make async operations work with metadata table #2332)
  • Changes the syncing model to only move over completed instants on data timeline
  • Syncing happens postCommit and on writeClient initialization
  • Latest delta commit on the metadata table is sufficient as the watermark for data timeline archival
  • Cleaning/Compaction use a suffix to the last instant written to metadata table, such that we keep the 1-1
  • .. mapping between data and metadata timelines.
  • Got rid of a lot of the complexity around checking for valid commits during open of base/log files
  • Tests now use local FS, to simulate more failure scenarios
  • Some failure scenarios exposed HUDI-1434, which is needed for MOR to work correctly

co-authored by: Vinoth Chandar [email protected]

Tips

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.

@vinothchandar vinothchandar added priority:blocker Production down; release blocker status:in-progress Work in progress labels Dec 31, 2020
@vinothchandar vinothchandar self-assigned this Dec 31, 2020
@codecov-io
Copy link

codecov-io commented Dec 31, 2020

Codecov Report

Merging #2398 (49bf0e0) into master (a23aa41) will decrease coverage by 1.86%.
The diff coverage is 17.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2398      +/-   ##
============================================
- Coverage     52.05%   50.18%   -1.87%     
- Complexity     2953     2982      +29     
============================================
  Files           395      410      +15     
  Lines         17652    18395     +743     
  Branches       1809     1884      +75     
============================================
+ Hits           9188     9231      +43     
- Misses         7706     8405     +699     
- Partials        758      759       +1     
Flag Coverage Δ Complexity Δ
hudicli 37.28% <1.12%> (-1.56%) 0.00 <1.00> (ø)
hudiclient 100.00% <ø> (ø) 0.00 <ø> (ø)
hudicommon 51.71% <14.83%> (-3.28%) 0.00 <55.00> (ø)
hudiflink 10.20% <ø> (ø) 0.00 <ø> (ø)
hudihadoopmr 33.35% <82.50%> (-0.18%) 0.00 <5.00> (ø)
hudisparkdatasource 62.81% <50.00%> (-0.05%) 0.00 <0.00> (ø)
hudisync 48.71% <86.66%> (+0.54%) 0.00 <2.00> (ø)
huditimelineservice 65.30% <ø> (ø) 0.00 <ø> (ø)
hudiutilities 69.65% <100.00%> (ø) 0.00 <1.00> (ø)

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

Impacted Files Coverage Δ Complexity Δ
...pache/hudi/common/config/HoodieMetadataConfig.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...c/main/java/org/apache/hudi/common/fs/FSUtils.java 45.23% <0.00%> (-2.03%) 54.00 <0.00> (ø)
...di/common/table/timeline/HoodieActiveTimeline.java 69.62% <ø> (ø) 41.00 <0.00> (ø)
.../hudi/common/table/view/FileSystemViewManager.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...va/org/apache/hudi/metadata/BaseTableMetadata.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...e/hudi/metadata/FileSystemBackedTableMetadata.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...pache/hudi/metadata/HoodieBackedTableMetadata.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...he/hudi/metadata/HoodieMetadataFileSystemView.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...data/HoodieMetadataMergedInstantRecordScanner.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...metadata/HoodieMetadataMergedLogRecordScanner.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
... and 53 more

@vinothchandar
Copy link
Member Author

@prashantwason @rmpifer @umehrot2 PR against master ready to go. Please take a pass at parts you have all worked on

prashantwason and others added 5 commits December 31, 2020 09:04
 - Introduced an internal metadata table, that stores file listings.
 - metadata table is kept upto date with
 - Fixed handling of CleanerPlan.
 - [HUDI-842] Reduce parallelism to speed up the test.
 - [HUDI-842] Implementation of CLI commands for metadata operations and lookups.
 - [HUDI-842] Extend rollback metadata to include the files which have been appended to.
 - [HUDI-842] Support for rollbacks in MOR Table.
 - MarkerBasedRollbackStrategy needs to correctly provide the list of files for which rollback blocks were appended.
 - [HUDI-842] Added unit test for rollback of partial commits (inflight but not completed yet).
 - [HUDI-842] Handled the error case where metadata update succeeds but dataset commit fails.
 - [HUDI-842] Schema evolution strategy for Metadata Table. Each type of metadata saved (FilesystemMetadata, ColumnIndexMetadata, etc.) will be a separate field with default null. The type of the record will identify the valid field. This way, we can grow the schema when new type of information is saved within in which still keeping it backward compatible.
 - [HUDI-842] Fix non-partitioned case and speedup initial creation of metadata table.Choose only 1 partition for jsc as the number of records is low (hundreds to thousands). There is more overhead of creating large number of partitions for JavaRDD and it slows down operations like WorkloadProfile.
For the non-partitioned case, use "." as the name of the partition to prevent empty keys in HFile.
 - [HUDI-842] Reworked metrics pusblishing.
 - Code has been split into reader and writer side. HoodieMetadata code to be accessed by using HoodieTable.metadata() to get instance of metdata for the table.
Code is serializable to allow executors to use the functionality.
 - [RFC-15] Add metrics to track the time for each file system call.
 - [RFC-15] Added a distributed metrics registry for spark which can be used to collect metrics from executors. This helps create a stats dashboard which shows the metadata table improvements in real-time for production tables.
 - [HUDI-1321] Created HoodieMetadataConfig to specify configuration for the metadata table. This is safer than full-fledged properties for the metadata table (like HoodieWriteConfig) as it makes burdensome to tune the metadata. With limited configuration, we can control the performance of the metadata table closely.

[HUDI-1319][RFC-15] Adding interfaces for HoodieMetadata, HoodieMetadataWriter (apache#2266)
 - moved MetadataReader to HoodieBackedTableMetadata, under the HoodieTableMetadata interface
 - moved MetadataWriter to HoodieBackedTableMetadataWriter, under the HoodieTableMetadataWriter
 - Pulled all the metrics into HoodieMetadataMetrics
 - Writer now wraps the metadata, instead of extending it
 - New enum for MetadataPartitionType
 - Streamlined code flow inside HoodieBackedTableMetadataWriter w.r.t initializing metadata state
 - [HUDI-1319] Make async operations work with metadata table (apache#2332)
 - Changes the syncing model to only move over completed instants on data timeline
 - Syncing happens postCommit and on writeClient initialization
 - Latest delta commit on the metadata table is sufficient as the watermark for data timeline archival
 - Cleaning/Compaction use a suffix to the last instant written to metadata table, such that we keep the 1-1
 - .. mapping between data and metadata timelines.
 - Got rid of a lot of the complexity around checking for valid commits during open of base/log files
 - Tests now use local FS, to simulate more failure scenarios
 - Some failure scenarios exposed HUDI-1434, which is needed for MOR to work correctly

co-authored by: Vinoth Chandar <[email protected]>
…apache#2326)

[HUDI-1394] [RFC-15] Use metadata table (if present) to get all partition paths (apache#2351)
…d listing. (apache#2343)

 * [HUDI-1469] Faster initialization of metadata table using parallelized listing which finds partitions and files in a single scan.
 * MINOR fixes

Co-authored-by: Vinoth Chandar <[email protected]>
…able (apache#2342)

[RFC-15] Fix partition key in metadata table when bootstrapping from file system (apache#2387)

Co-authored-by: Ryan Pifer <[email protected]>
@vinothchandar
Copy link
Member Author

@prashantwason tests pass . I m trying to get MOR tests running again in parallel

Copy link
Member

Choose a reason for hiding this comment

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

this should be within another if block:

if(useFileListingFromMetadata) {
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is a fallback code inside HoodieBackedTableMetadata. but yeah I ll add one to HoodieTableMetadata#create() to return a different impl when useFileListingFromMetadata=false

Copy link
Member Author

Choose a reason for hiding this comment

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

I will look at this holistically during https://issues.apache.org/jira/browse/HUDI-1479

Copy link
Member

Choose a reason for hiding this comment

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

This can be from FileSystemBackedTableMetadata.getAllFilesInPartition(...) for symmetry with above method.

Copy link
Member Author

Choose a reason for hiding this comment

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

So there is the issue of recursing, let me see how to make it work.

Comment on lines +95 to +96
Copy link
Contributor

Choose a reason for hiding this comment

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

should also add REPLACE_COMMIT_ACTION here

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 Are there other new actions we need to be taking into account as well i.e. replace, clustering?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. https://issues.apache.org/jira/browse/HUDI-1459 tracks this. and is still a release blocker

@vinothchandar
Copy link
Member Author

@rmpifer @prashantwason I ran into an issue trying to make MOR work. Essentially the check we added to skip rollback instants if their original instants have not been applied (discussion here #2342 (comment) ) does not take into account the fact that for MOR tables, restore can add new log files to the table, to record the fact that this instant was rolled back. So tests fail since those logs are missed. Trying to find a cleaner fix. Will fix, get tests to pass before merging this.

Copy link
Contributor

Choose a reason for hiding this comment

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

after https://github.com/apache/hudi/pull/2136/files merged, we will reuse the metaclient in AbstractHoodieWriteClient.java

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

Comment on lines +75 to +84
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this make sense to have this configurability in CLI? During write operations this field is not configurable so metadata will always be updated at tableBasePath + '.hoodie/metadata/'

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @prashantwason to follow on, with this thinking

Copy link
Contributor

@rmpifer rmpifer Jan 3, 2021

Choose a reason for hiding this comment

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

[Minor] Not sure what purpose of readonly config is here. This command doesn't return any values. This command seems like its only purpose is to perform a write operation

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @prashantwason again

Comment on lines +278 to +282
Copy link
Contributor

Choose a reason for hiding this comment

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

[Minor] We could dedup repeated logic by using HoodieTableFileSystemView.createInMemoryFileSystemView

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm. good point. I am not sure if that will work well, with timeline server etc. best to keep it like this for now.

Comment on lines +249 to +253
Copy link
Contributor

Choose a reason for hiding this comment

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

[Minor] getFileSystemViewInternal essentially does the same, could we not just directly call here

Copy link
Member Author

Choose a reason for hiding this comment

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

same as above. tracking this for a review again in HUDI-1479

Copy link
Contributor

Choose a reason for hiding this comment

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

[Minor] Rather than listing we should be able to just call delete on path if exists

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @prashantwason might be good to collect these in a JIRA and fix in a follow on?

Copy link
Contributor

Choose a reason for hiding this comment

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

[Minor] Maybe rename metadata sync based on the information provided in help. This command does not initialize the metadata

Comment on lines +464 to 481
Copy link
Contributor

Choose a reason for hiding this comment

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

[Minor] Possible to remove these unneeded indentations

Copy link
Member Author

Choose a reason for hiding this comment

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

we should just automate this IMO. long pending. Our checkstyle does not enforce wrapping and IDEs go their own thing. We can chat offline, if you are interested. cc @xushiyan as well, who was thinking of running point on this

Copy link
Member

Choose a reason for hiding this comment

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

@rmpifer @vinothchandar yup i'm looking into spotless setup, which does spotless:apply as part of a maven phase. and the ambiguity shall be resolved with a proper style configured.

Comment on lines +95 to +96
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 Are there other new actions we need to be taking into account as well i.e. replace, clustering?

…ynced as well

 - TestHoodieBackedMetadata#testSync etc now run for MOR tables
 - HUDI-1502 is still pending and has issues for MOR/rollbacks
 - Also addressed bunch of code review comments.
Copy link
Member Author

@vinothchandar vinothchandar left a comment

Choose a reason for hiding this comment

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

Planning to merge after CI passes.

Follow ups : HUDI-1502 , HUDI-1479

@vinothchandar vinothchandar merged commit 31e674e into apache:master Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:blocker Production down; release blocker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants