ad1happy2go commented on code in PR #19248:
URL: https://github.com/apache/hudi/pull/19248#discussion_r3568253339
##########
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)) {
Review Comment:
Done in f72a22e. Dropped the `contains()` guard — as you noted,
`setDefaults(...)` writes the `""` default for `META_SYNC_BASE_PATH` two lines
earlier, so `contains()` is always true and `getString()` can't be null at that
point. The `isNullOrEmpty` check is now the only guard, exactly as suggested.
##########
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:
Thanks for the detailed trace — agreed the code is safe but the claim was
not. Fixed in f72a22e:
- Reworded the PR description and the "left unchanged" test comment: a path
is unchanged only when it is already canonical (non-empty authority, single
slashes).
- Added a `file:///tmp/tbl` -> `file:/tmp/tbl` case to
`testNormalizeBasePath`, so the empty-authority reshape is pinned deliberately.
- Called out the reshape (and why it's harmless, via
`comparePathsWithoutScheme` re-parsing) in the constructor comment.
--
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]
