wombatu-kun commented on code in PR #19248:
URL: https://github.com/apache/hudi/pull/19248#discussion_r3566187341
##########
hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/HoodieSyncConfig.java:
##########
@@ -211,6 +212,16 @@ public HoodieSyncConfig(Properties props, Configuration
hadoopConf) {
.collect(Collectors.joining("\n")));
setDefaults(HoodieSyncConfig.class.getName());
this.hadoopConf = hadoopConf;
+ // Normalize the table base path so that downstream metastore LOCATION
values never carry
+ // consecutive or trailing slashes. Strict object-store filesystems (e.g.
GCS) reject "//",
+ // and this keeps the synced location consistent with the Hudi table base
path (which is
+ // itself normalized through org.apache.hadoop.fs.Path on the write path).
+ if (contains(META_SYNC_BASE_PATH)) {
+ String basePath = getString(META_SYNC_BASE_PATH);
+ if (!StringUtils.isNullOrEmpty(basePath)) {
+ setValue(META_SYNC_BASE_PATH, new Path(basePath).toString());
Review Comment:
Confirmed, and it is already pinned in-repo: hudi-io's TestStoragePath
asserts that `new StoragePath("file:///a/b/c").toString()` is `file:/a/b/c`.
Hadoop `Path` parses the empty authority the same way (`toString` emits `//`
only when `uri.getAuthority() != null`), so `file:///x` and `hdfs:///x` really
are reshaped.
The blast radius is wider than newly created tables:
`HoodieHiveSyncClient#updateLastCommitTimeSynced` calls
`sd.setLocation(config.getString(META_SYNC_BASE_PATH))` and
`serdeInfo.putToParameters(TABLE_SERDE_PATH, basePath)` on every sync, so
pre-existing HMS tables whose LOCATION is `file:///...` get rewritten to the
`file:/...` form on the next sync as well.
It still looks harmless. The table-location drift check in `HiveSyncTool`
goes through `FSUtils.comparePathsWithoutScheme`, which re-parses both operands
with `Path`, so the two forms compare equal and no table is dropped and
recreated. `HoodieSyncClient#getPartitionEvents` strips scheme and authority on
both sides, so no spurious partition UPDATE events either. Glue and BigQuery
never read this config value for the location at all: they use
`HoodieSyncClient#getBasePath()`, i.e. `metaClient.getBasePath()`, which is a
`StoragePath` and already yields the `file:/` form.
So the code is fine but the claim is not: "No behavior change for already
well-formed paths" in the description, and "A well-formed base path is left
unchanged." in `TestHoodieSyncConfig`, are both false for empty-authority URIs.
Worth rewording both and adding a `file:///tmp/tbl` -> `file:/tmp/tbl` case to
`testNormalizeBasePath` so the reshape is pinned deliberately instead of being
discovered later.
--
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]
