wombatu-kun commented on code in PR #19248:
URL: https://github.com/apache/hudi/pull/19248#discussion_r3568455868
##########
hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/HoodieSyncConfig.java:
##########
@@ -211,6 +212,18 @@ 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).
Note that
+ // empty-authority URIs such as file:///x and hdfs:///x are reshaped to
their canonical
+ // single-slash form (file:/x); this is harmless and consistent with the
write-path base path.
+ // setDefaults() above always writes the "" default for
META_SYNC_BASE_PATH, so the empty
+ // check is the only guard needed here.
+ String basePath = getString(META_SYNC_BASE_PATH);
+ if (!StringUtils.isNullOrEmpty(basePath)) {
+ setValue(META_SYNC_BASE_PATH, new Path(basePath).toString());
Review Comment:
`HiveTestUtil.basePath` is
`Files.createTempDirectory(...).toUri().toString()`, i.e.
`file:///tmp/hivesynctestNNN/`, and
`HoodieHiveSyncClient#updateLastCommitTimeSynced` writes the serde `path` from
`META_SYNC_BASE_PATH` on every sync. It now becomes
`file:/tmp/hivesynctestNNN`, so the `'path'='<basePath>'` assertions in
`TestHiveSyncTool#testSyncCOWTableWithProperties` and
`#testSyncMORTableWithProperties` fail in every parameterization (verified
locally on this head: 24/24 fail, and 24/24 pass with the normalization
reverted). These tests run in Azure, not in the GitHub Actions matrix, so the
green checks here do not cover them.
Update both expectations to the normalized value; that is also the only
end-to-end check of what reaches the metastore.
##########
hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/HoodieSyncConfig.java:
##########
@@ -211,6 +212,18 @@ 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).
Note that
Review Comment:
The write path normalizes via `StoragePath` (`HoodieTableMetaClient`), not
`org.apache.hadoop.fs.Path`; `HoodieWriteConfig#getBasePath` returns the raw
string.
Use `new StoragePath(basePath).toString()`: it makes the claim true and
strips trailing slashes fully, whereas hadoop `Path` on the default
`hadoop.version` (2.10.2) turns `gs://b/t///` into `gs://b/t/`. `StoragePath`
is already used in hudi-sync-common (`HoodieSyncClient`).
--
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]
