szehon-ho commented on code in PR #7361:
URL: https://github.com/apache/iceberg/pull/7361#discussion_r1174188399
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -343,8 +344,12 @@ private Result doExecuteWithPartialProgress(
.executeWith(rewriteService)
.noRetry()
.onFailure(
- (fileGroup, exception) ->
- LOG.error("Failure during rewrite group {}", fileGroup.info(),
exception))
+ (fileGroup, exception) -> {
Review Comment:
I'm wondering if we can't avoid touching the commitService. It doesnt seem
like its really the responsibility of the commit service to handle the failed
ones. Can't this method just keep a list internally? ie,
```
List<Failure> failures;
...
onFailure(
(fileGroup, exception) -> {
LOG.error("...")
failures.addFailure(fileGroup, exception)
}
```
##########
api/src/main/java/org/apache/iceberg/actions/RewriteDataFiles.java:
##########
@@ -197,6 +197,11 @@ default int rewrittenDataFilesCount() {
default long rewrittenBytesCount() {
return
rewriteResults().stream().mapToLong(FileGroupRewriteResult::rewrittenBytesCount).sum();
}
+
+ @Value.Default
Review Comment:
Would it make more sense to have a FailedFileGroupRewriteResult, with just
failure? Can also put exception message there then? cc @aokolnychyi
@RussellSpitzer for thoughts
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
