amogh-jahagirdar commented on code in PR #16686:
URL: https://github.com/apache/iceberg/pull/16686#discussion_r3507110598
##########
core/src/test/java/org/apache/iceberg/TestSnapshotSummary.java:
##########
@@ -212,6 +220,86 @@ public void deleteWithDuplicates() {
.containsEntry(SnapshotSummary.REPLACED_MANIFESTS_COUNT, "1");
}
+ @TestTemplate
+ public void deleteDuplicateFilesWithinMultipleManifests() throws Exception {
Review Comment:
Thanks @HuaHuaY see my comment
https://github.com/apache/iceberg/pull/16686#r3507100091 on my thoughts on the
concurrency tests. the TLDR is I think i'd just remove the 2 concurrency
focused tests now that we've structurally made the internal state for this path
for collecting the files to delete after the parallel manifest read to be
single threaded. The tests are more complexity than they're really worth imo.
##########
core/src/test/java/org/apache/iceberg/TestOverwrite.java:
##########
@@ -335,4 +336,55 @@ public void testValidatedOverwriteWithAppendSuccess() {
assertThat(latestSnapshot(base, branch).snapshotId()).isEqualTo(baseId);
}
+
+ @TestTemplate
+ public void testFullOverwriteAcrossManyManifests() {
Review Comment:
While this and the below concurrency test are well intentioned I think the
issue with these kinds of tests is that we end up doing a lot of complex stuff
just to try and repro the internal state of that class that used to be
concurrent. We could use barriers but even that's super complex.
If we take a step back, in terms of the internal implementation we've now
structurally changed the code to collect the deleted files in a single-threaded
manner; so it's a bit questionable to write a test which does all this work to
simulate the conditions to hit this internal state that no longer exists.
In combination with the deterministic test for making sure retries don't
double count the duplicates, I think we're fairly well covered.
In that case, I would simply just recommend removing this and the test
below.
--
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]
