[DISCUSS] getInitialTable table cache: Hive and Glue sync clients disagree on whether a cached read may hide the client's own write
- Dominant language
- Java
- Stars
- 6.2k
- Forks
- 2.5k
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 112
Description
### Problem
`HoodieHiveSyncClient` and `AWSGlueCatalogSyncClient` both memoize the metastore table in a per-instance `initialTableByName` map, filled lazily by `getInitialTable`. It was added in HUDI-9365 ("Reduce overhead of Hive and AWS Glue sync tools") to avoid repeated `getTable` calls during a sync. In both clients exactly three reads go through it: `getLastCommitTimeSynced`, `getLastCommitCompletionTimeSynced` and `getTableLocation`.
Neither client invalidates that entry when it mutates the table. In `HoodieHiveSyncClient` eight methods call `alter_table` or `dropTable` (`updateTableProperties`, `updateSerdeProperties`, `createOrReplaceTable`, `updateLastReplicatedTimeStamp`, `deleteLastReplicatedTimeStamp`, `updateLastCommitTimeSynced`, `updateHoodieWriterVersion`, `dropTable`) and none of them touch the map. So within a single sync run the client can hand back a `last_commit_time_sync` or a table location that it has itself already overwritten.
The two clients then diverge, by accident rather than by design. `AWSGlueCatalogSyncClient.tableExists` performs a real `GetTable` and re-puts the result into `initialTableByName`, and `HiveSyncTool.syncHoodieTable` calls `tableExists` as its very first step, so on Glue the entry is refreshed immediately before every cached read. `HoodieHiveSyncClient.tableExists` returns the metastore client's boolean and never populates the map, so on Hive the entry survives from the first `getTableLocation` of the run until the client is closed.
### Why it is latent today rather than live
The one path that exercised the divergence was `HiveSyncTool.doSync()` under the default `ALL` strategy with `hoodie.datasource.hive_sync.skip_ro_suffix=true`. `initTableNameVars` makes `roTableName` the bare table name, so the same metastore table was synced twice in one run, first as read-optimized and then again by the "sync origin table" step. On Hive the second call read the pre-write snapshot, `isAlreadySynced` returned false, and `updateSerdeProperties` rewrote the input format to `HoodieParquetRealtimeInputFormat`; the refreshed entry on Glue would have made the same sequence short-circuit instead. That is what #16637 and #12011 report, and the open PR #19427 fixes it by removing the redundant second sync.
With #19427 in, I could not find any remaining path in the Hive sync code that reads a cached field after writing it. `GlobalHiveSyncTool` reads replication timestamps live rather than through `getInitialTable`, `recreateAndSyncHiveTable` never re-reads after replacing the table, and every other entry point builds a fresh `HiveSyncTool` per sync. So this is currently an inconsistency between two implementations of the same interface rather than a reproducible failure.
### The design question
Which contract is `getInitialTable` meant to have? The two answers imply opposite patches, and I do not think the code settles it.
1. **A per-run snapshot**, as the name suggests: the table as it was when this sync started. Under this reading `isAlreadySynced` is asking "was this table already up to date before I began", which is a reasonable question, `HoodieHiveSyncClient` is correct as written, and the defect is `AWSGlueCatalogSyncClient.tableExists` quietly breaking the snapshot on every call.
2. **A cache of current state.** Under this reading Glue is correct and `HoodieHiveSyncClient` should invalidate or refresh the entry in the methods that mutate the fields it serves.
### Proposal
Pick one of the two readings, make both clients honor it, and record the choice in a comment on `getInitialTable`, because the next person who syncs one table twice in a run will hit whichever half is wrong. If (2) is chosen, the smallest change is a private `invalidate(tableName)` called from `updateLastCommitTimeSynced`, `createOrReplaceTable` and `dropTable`, the three mutators that change the cached fields, which keeps HUDI-9365's one-fetch-per-table behavior on the common path. Either way a client-level test that reads, writes and reads again would pin the chosen semantics.
Happy to send the patch once the direction is settled.
### Appendix: how this surfaced
Reviewing PR #19427. Its regression test syncs once against an empty metastore, which is the one case where the redundant real-time sync is skipped anyway by the `isAlreadySynced` guard, so the test passes with the production change reverted. Adding a second commit and another sync round makes it fail with `expected: but was: `. Working out why one round behaves differently from two is what surfaced the cache.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with getInitialTable and the caching behavior in HoodieHiveSyncClient and AWSGlueCatalogSyncClient, then review the three cached reads and the listed table-mutating methods. Compare tableExists in both clients and the behavior described around PR #19427. Done means both clients follow one documented cache contract and a client-level read-write-read test pins the chosen semantics.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- aws, java
- Domain
- data-engineering, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100