davidradl commented on code in PR #26498:
URL: https://github.com/apache/flink/pull/26498#discussion_r2054878468
##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/sink/filesystem/Buckets.java:
##########
@@ -85,6 +92,8 @@ public class Buckets<IN, BucketID> {
private final BucketStateSerializer<BucketID> bucketStateSerializer;
+ private final ExecutorService snapshotActiveBucketsThreadPool;
Review Comment:
some comments:
- I am not convinced that this is covered by existing tests as there is a
new thread pool that has been introduced.
- Also I would like to see a test showing the expected behaviour when the
CompletionException is thrown. I am unsure what we are looking to do when there
is a CompletionException is thrown. Does everything get cleaned up when this
exception is thrown?
- the title talks of close buckets but the code is in snapshotActiveBuckets.
It would be easier to understand if I could see an obvious connection between
close buckets and snapshotActiveBuckets.
- I see the Jira talks about introducing an option for this - but the code
has not done this - it has changed it from synchronous to asynchronous. what is
the thinking here?
- the CI has failed - I am not sure if it relates to this change.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
