You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@xtable.apache.org by "parisni (via GitHub)" <gi...@apache.org> on 2026/02/03 10:52:18 UTC
[PR] feat: cli to fail on sync error [incubator-xtable]
parisni opened a new pull request, #794:
URL: https://github.com/apache/incubator-xtable/pull/794
## Important Read
- GitHub issue: TBD
## What is the purpose of the pull request
This pull request adds a --failOnError CLI option to make catalog/table sync fail the process when any sync fails.
## Brief change log
- Added failOnError CLI flag to RunCatalogSync.
- Propagated sync failures based on SyncResult status codes.
## Verify this pull request
This pull request is already covered by existing tests, such as existing unit tests for catalog sync utilities (not run).
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: cli to fail on sync error [incubator-xtable]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #794:
URL: https://github.com/apache/incubator-xtable/pull/794#discussion_r3526076200
##########
xtable-utilities/src/main/java/org/apache/xtable/utilities/RunSync.java:
##########
@@ -183,9 +192,16 @@ static void syncTableMetdata(
.syncMode(SyncMode.INCREMENTAL)
.build();
try {
- conversionController.sync(conversionConfig, conversionSourceProvider);
+ Map<String, SyncResult> syncResults =
+ conversionController.sync(conversionConfig, conversionSourceProvider);
+ if (failOnError && hasSyncFailures(syncResults)) {
+ throw new RuntimeException("Sync completed with failures. See logs for details.");
+ }
} catch (Exception e) {
log.error("Error running sync for {}", table.getTableBasePath(), e);
+ if (failOnError) {
+ throw e;
Review Comment:
It looks like the caller is also catching an exception so lets take this opportunity to clean this up and only catch once.
##########
xtable-utilities/src/main/java/org/apache/xtable/utilities/RunCatalogSync.java:
##########
@@ -190,15 +200,38 @@ public static void main(String[] args) throws Exception {
.distinct()
.collect(Collectors.toList());
try {
- conversionController.syncTableAcrossCatalogs(
- conversionConfig,
- getConversionSourceProviders(tableFormats, tableFormatConverters, hadoopConf));
+ Map<String, SyncResult> syncResults =
+ conversionController.syncTableAcrossCatalogs(
+ conversionConfig,
+ getConversionSourceProviders(tableFormats, tableFormatConverters, hadoopConf));
+ if (failOnError && hasSyncFailures(syncResults)) {
+ throw new RuntimeException("Sync completed with failures. See logs for details.");
+ }
} catch (Exception e) {
log.error("Error running sync for {}", sourceTable.getBasePath(), e);
+ if (failOnError) {
+ throw e;
+ }
}
}
}
+ private static boolean hasSyncFailures(Map<String, SyncResult> syncResults) {
+ return syncResults.values().stream().anyMatch(RunCatalogSync::syncResultHasFailure);
+ }
+
+ private static boolean syncResultHasFailure(SyncResult syncResult) {
Review Comment:
Can we make this an instance method on the `SyncResult` instead? Then we can avoid duplicating the logic between the two files in this PR.
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: cli to fail on sync error [incubator-xtable]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #794:
URL: https://github.com/apache/incubator-xtable/pull/794#discussion_r2776712037
##########
xtable-utilities/src/main/java/org/apache/xtable/utilities/RunCatalogSync.java:
##########
@@ -190,15 +200,38 @@ public static void main(String[] args) throws Exception {
.distinct()
.collect(Collectors.toList());
try {
- conversionController.syncTableAcrossCatalogs(
- conversionConfig,
- getConversionSourceProviders(tableFormats, tableFormatConverters, hadoopConf));
+ Map<String, SyncResult> syncResults =
+ conversionController.syncTableAcrossCatalogs(
+ conversionConfig,
+ getConversionSourceProviders(tableFormats, tableFormatConverters, hadoopConf));
+ if (failOnError && hasSyncFailures(syncResults)) {
Review Comment:
Should we add similar functionality to `RunSync`?
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: cli to fail on sync error [incubator-xtable]
Posted by "parisni (via GitHub)" <gi...@apache.org>.
parisni commented on code in PR #794:
URL: https://github.com/apache/incubator-xtable/pull/794#discussion_r2793866958
##########
xtable-utilities/src/main/java/org/apache/xtable/utilities/RunCatalogSync.java:
##########
@@ -190,15 +200,38 @@ public static void main(String[] args) throws Exception {
.distinct()
.collect(Collectors.toList());
try {
- conversionController.syncTableAcrossCatalogs(
- conversionConfig,
- getConversionSourceProviders(tableFormats, tableFormatConverters, hadoopConf));
+ Map<String, SyncResult> syncResults =
+ conversionController.syncTableAcrossCatalogs(
+ conversionConfig,
+ getConversionSourceProviders(tableFormats, tableFormatConverters, hadoopConf));
+ if (failOnError && hasSyncFailures(syncResults)) {
Review Comment:
@the-other-tim-brown good point. done
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org