Copilot commented on code in PR #15226:
URL: https://github.com/apache/iceberg/pull/15226#discussion_r2777585644
##########
core/src/main/java/org/apache/iceberg/actions/BaseCommitService.java:
##########
@@ -159,6 +161,14 @@ public List<T> results() {
return Lists.newArrayList(committedRewrites.iterator());
}
+ /** Returns all File groups which failed to commit */
+ public List<T> failures() {
+ Preconditions.checkState(
+ committerService.isShutdown(),
+ "Cannot get failures from a service which has not been closed");
+ return Lists.newArrayList(failedRewrites.iterator());
+ }
Review Comment:
The new Javadoc and precondition message are a bit unclear/grammatically
awkward. Consider updating to clarify that these are file groups from commit
batches that threw, and tweak wording/capitalization (e.g., “file groups that
failed to commit” and “service that has not been closed”).
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java:
##########
@@ -1392,6 +1392,8 @@ public void
testParallelPartialProgressWithCommitFailure() {
// Commit 1: 4/4 + Commit 2 failed 0/4 + Commit 3: 2/2 == 6 out of 10
total groups committed
assertThat(result.rewriteResults()).as("Should have 6
fileGroups").hasSize(6);
+ assertThat(result.rewriteFailures()).hasSize(4);
+ assertThat(result.failedDataFilesCount()).isEqualTo(8);
Review Comment:
The newly added assertions don’t include an `.as(...)` description, which
can make failures harder to diagnose (especially since the test already uses
descriptions for other expectations). Consider adding assertion descriptions
for these two checks (and applying the same pattern in the corresponding Spark
v4.0/v3.5/v3.4 test updates).
```suggestion
assertThat(result.rewriteFailures()).as("Should have 4 failed
fileGroups").hasSize(4);
assertThat(result.failedDataFilesCount()).as("Should have 8 failed data
files").isEqualTo(8);
```
--
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]
