lindong28 commented on PR #21690: URL: https://github.com/apache/flink/pull/21690#issuecomment-1384878158
Hi @1996fanrui, thanks for the explanation. 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, unless one implementation is considerably much simpler and easier to read (which is probably not the case here). 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. But I am open for the approach in this PR if both you and @pnowojski prefer this. -- 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
