liucao-dd commented on code in PR #16980:
URL: https://github.com/apache/iceberg/pull/16980#discussion_r3487138370
##########
core/src/test/java/org/apache/iceberg/TestDeleteFiles.java:
##########
@@ -738,6 +740,32 @@ public void
removingDataFilesWhenTruncatingAlsoRemovesDVs() {
statuses(Status.DELETED, Status.DELETED));
}
+ @TestTemplate
+ public void testConcurrentManifestFilteringWithExpression() {
Review Comment:
Thanks for picking this up! Since I opened #16978 yesterday and also opened
#16984 with a different fix, want to acknowledge that your approach is
lock-free on the hot path and has higher throughput potential.
I was thinking that in object-store-backed deployments, each manifest is
read from S3/GCS, so millisecond-scale I/O per manifest dominates both the
microseconds of CPU and the nanoseconds of lock. Hence I went with a simpler
one-liner fix to use synchronizedSet.
The one thing I’d suggest is strengthening the regression test. With only 4
manifests / 4 files, the test may pass even without the fix because it does not
create enough concurrent pressure on the underlying `LinkedHashSet`. In #16984
I used many files across many manifests with colliding file-location hashes to
make the unfixed race much easier to reproduce. I’ll defer to maintainers on
which implementation they prefer, but wanted to share this since we’re
addressing the same issue.
--
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]
