amogh-jahagirdar commented on code in PR #16686:
URL: https://github.com/apache/iceberg/pull/16686#discussion_r3507100091
##########
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 but no longer is. 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]
