wombatu-kun opened a new issue, #19431:
URL: https://github.com/apache/hudi/issues/19431
### 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: <org.apache.hudi.hadoop.HoodieParquetInputFormat> but was:
<org.apache.hudi.hadoop.realtime.HoodieParquetRealtimeInputFormat>`. Working
out why one round behaves differently from two is what surfaced the cache.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
