1996fanrui commented on PR #21690: URL: https://github.com/apache/flink/pull/21690#issuecomment-1384889755
Hi @lindong28 , thanks for your quick feedback. I want to check with you the first question: No matter which class canEmitBatchOfRecords is added to, we should use `Supplier<Boolean> canEmitBatchOfRecords` instead of `Supplier<Boolean> mailboxHasMail;` due to `canEmitBatchOfRecords` should add some other checks and it's more general and abstract, right? If yes, we can focus on the next question: Where is `canEmitBatchOfRecords` added to? > Regarding the 1st point mentioned above, yes `canEmitBatchOfRecords()` is related to emit int the sense that it mangaes the "control flow" of when emit can be called. So it is probably consistent with my previous comment. > > Regarding the 2nd point, I guess you are saying the implementation of this "optimization" after the refactor is simpler. IMO the abstraction (or separate of concern) between classes is generally more important than implementation, because good abstraction makes code more intuitive and easier to maintain in the long term. I suppose the difference between the complexity of these two implementations is not significant in this case. > > I guess ultimately it comes down to whether the extra implementation simplicity is more important than keeping the DataOutput focusing only on data flow. I am inclined to keep the existing abstraction. Sounds make sense. Let's wait for @pnowojski 's feedback. -- 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
