rmpifer opened a new pull request #2342: URL: https://github.com/apache/hudi/pull/2342
## *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 Currently every time an operation (i.e. commit, clean, rollback) completes we attempt to keep the metadata table up to date by creating an upsert on the metadata table with the same instant time. This way we can reference both datasets timelines and directly compare the instants on them to see if they are in sync. There can be the possibility that the dataset timeline and the metadata table timeline become out of sync. When trying to read from the metadata table while the timeline is out of sync you would get incorrect values for `getAllFilesInPartition` and `getAllPartitionPaths`. This change provides a way to overcome this scenario by reading unsynced timeline instants and merging it with existing metadata table records to get the most up to date state of the file system JIRA: https://issues.apache.org/jira/browse/HUDI-1325 ## Brief change log #### Timeline Sync * The logic of converting timeline metadata to metadata table records was directly tied to the commit phase in `HoodieBackedMetadataWriter`. Refactored this logic to a utility class `HoodieTableMetadataTimelineUtil` * Created a scanner `HoodieMetadataMergedInstantRecordScanner` which handles conversion of timeline instants to metadata records and merges results * Added final step in `AbstractHoodieTableMetadata.getMergedRecordByKey` which uses the new scanner mentioned to fetch the `HoodieRecord` associated with the desired `key` from the unsynced timeline instants and merge it with the record from the metadata table * When converting rollback operation to metadata table records there was logic that re-read from the metadata table to ensure any files being deleted as part of roll back existed. ``` // Rollbacks deletes instants from timeline. The instant being rolled-back may not have been synced to the // metadata table. Hence, the deleted filed need to be checked against the metadata. ``` This doesn't make sense since all instants are processed in serial order so there would never be the case where a rollback was being written before an instant earlier on the timeline was already synced. Removed this logic because it created circular dependency when implementing timeline merging * Changed the validate metadata step in tests to use the metadata reader `HoodieBackedTableMetadata`. By default when metadata writer `HoodieBackedTableMetadataWriter` is initialized it syncs all instants to the metadata table. By using the reader we can simulate metadata table being out of sync. * Modified `initMetaClient` in test base class to allow table type to be passed in since table type is always set as `COPY_ON_WRITE` if using this method to initialize the meta client #### Refactor For the following reasons I modified the HoodieTableMetadata interface to be an AbstractHoodieMetadata class which contains the following shared functionality irresepective of how the metadata is stored (as a Hoodie table, in some key/value store): * Fetching `getAllPartitions` and `getAllFileInPartition` from metadata should validate metadata flag is enabled and should default to file listing if any error occurs regardless of storage type * During `fetchAllPartitions` and `fetchAllFilesInPartition` metrics should be published on operations and if `validateLookUps` is enabled, results returned from metadata should be compared against actual file listing results regardless of what storage type is * In `getMergedRecordByKey` regardless of how the key is fetched from storage the result is merged against unsynced timeline instants. This is why I introduced abstract `getRecordByKeyFromMetadata` which can be implemented by inheriting class which contains logic specific to storage type. * Moved `findInstantsToSync` to `AbstractHoodieTableMetadata`. This method is needed regardless of storage type to find if metadata is in sync with timeline. However since how the last synced instant is stored can be dependent on storage type made this method `abstract`. ## 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: ### Testing Ran `TestHoodieBackedMetadata` which contains all tests related to metadata table Added tests which use an unsynced client after commits, cleans, and restores have been performed to ensure updates not written to the metadata yet are still being reflected when reading from the metadata ## 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. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
