hudi-agent commented on code in PR #19248:
URL: https://github.com/apache/hudi/pull/19248#discussion_r3565769862
##########
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:
🤖 One subtlety: `new Path(...).toString()` drops the `//` when the URI
authority is empty, so an already well-formed path like `file:///tmp/tbl`
becomes `file:/tmp/tbl` (same for `hdfs:///path`). It's semantically equivalent
and Hive accepts both, but it does slightly contradict the "no behavior change
for well-formed paths" note and could show up as a LOCATION diff for existing
local/default-FS tables on re-sync. Have you confirmed that's harmless for your
target metastores?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
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:
🤖 nit: if `getString()` returns null for absent keys (which `isNullOrEmpty`
catches), the outer `contains` guard is redundant and adds an extra nesting
level. Could you simplify to just `if
(!StringUtils.isNullOrEmpty(getString(META_SYNC_BASE_PATH)))`?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
