1996fanrui commented on PR #21690: URL: https://github.com/apache/flink/pull/21690#issuecomment-1386442307
Hi @pnowojski @lindong28 , thanks for your discussion. > About the Suppliers, I don't like using them personally tbh. For one thing they are hard to follow in the IDE (finding implementations or usages do not work). This could be mitigated with using a named interface instead of Supplier<Boolean>. > > How about we add an internal functional interface in StreamTask? It could be similar to the existing SizeSupplier interface ([link](https://github.com/apache/flink/blob/master/flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TaskIOMetricGroup.java#L294)). I totally agree using a `named interface` instead of `Supplier<Boolean>`, and the `SizeSupplier` is a demo. How about this? ``` /** Check whether records can be emitted in batch. */ @FunctionalInterface public interface CanEmitBatchOfRecordsChecker { boolean check(); } ``` > The other thing that looks strange if I take a step back, is that we now have to different while (true) loops. One in the MailboxProcessor#runMailboxLoop the other in SourceOperator and with [FLINK-30709](https://issues.apache.org/jira/browse/FLINK-30709) in the network source as well, doing basically the same thing. Adding loops inside `SourceOperator` and `NetworkInput` is to reduce the call stack and avoid outer loops. The outer loop is still needed because `canEmitBatchOfRecords` may be false. If we are worried about other developers misunderstanding, we can add some comments to the loop of `SourceOperator` and `NetworkInput`. What do you think? -- 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
