amogh-jahagirdar commented on code in PR #18074:
URL: https://github.com/apache/iceberg/pull/18074#discussion_r4437438571
##########
core/src/test/java/org/apache/iceberg/TestTransaction.java:
##########
@@ -1119,6 +1119,91 @@ public void testExtendBaseTransaction() {
.hasSize(2);
}
+ @TestTemplate
+ public void testCommitTransactionDoesNotReadCommittedManifestLists() throws
IOException {
Review Comment:
I think this test is a bit redundant with
testTransactionCleanupReadsNoManifestLists. That test basically tests the same
behavior in a more complex scenario with a retry, and non-empty orphan file.
Generally I'd go for trying to keep the simplest test which validates the
behavior we desire, but that other test is also still a very realistic test, so
I'd recommend just dropping this one.
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -343,7 +343,7 @@ private WriterWrapper getWriter(Object key, int
partitionSpecId) {
}
@Override
- protected void cleanUncommitted(Set<ManifestFile> committed) {
+ protected void cleanUncommitted(Set<String> committed) {
Review Comment:
Thanks for the cleanup, I think it's a good idea to have changed this to a
String, generally we want to be passing around smallest structure required
(even though in the end it's just a reference, we can tell from the signature
that all we need for this is the path) for that operation.
##########
core/src/test/java/org/apache/iceberg/TestTransaction.java:
##########
@@ -683,7 +683,7 @@ public void
testTransactionRetryAndAppendManifestsWithSnapshotIdInheritance() th
}
@TestTemplate
- public void testTransactionCleanupReadsOnlyOwnManifestLists() throws
IOException {
+ public void testTransactionCleanupReadsNoManifestLists() throws IOException {
File location = java.nio.file.Files.createTempDirectory(temp,
"junit").toFile();
String tableName = "txnCleanupOwnManifestListsTest";
Review Comment:
minor, the table name should be changed now given the behavior we're testing.
##########
core/src/test/java/org/apache/iceberg/TestSnapshotProducer.java:
##########
@@ -403,4 +409,82 @@ public void monotonicityIsScopedToTargetBranch() {
.isEqualTo(mainNewTs)
.isLessThan(branchTs);
}
+
+ @TestTemplate
+ public void testCommitDoesNotReadCommittedManifestList() {
+ String tableName = "no-manifest-list-reread";
+ List<String> openedInputFiles = Lists.newArrayList();
+ TestTables.LocalFileIO countingFileIO =
+ new TestTables.LocalFileIO() {
Review Comment:
A few thoughts on the tests:
- I think we can drop the counting `FileIO` and just spy `LocalFileIO` like
`TestRemoveSnapshots.testAppendOnlyManifestsNotScannedDuringCleanup` does:
```java
TestTables.LocalFileIO spyFileIO = Mockito.spy(new TestTables.LocalFileIO());
...
testTable.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
Mockito.verify(spyFileIO, Mockito.never())
.newInputFile(testTable.currentSnapshot().manifestListLocation());
```
- I think the practice for new tests is to drop the `test` prefix in the
method name, e.g. `commitDoesNotReadCommittedManifestList`.
--
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]
