becketqin commented on PR #23876: URL: https://github.com/apache/flink/pull/23876#issuecomment-1846311079
@pvary Thanks for the patch. I am wondering if you have compared this approach with the alternative I mentioned in the mailing list. > Option 1: >1. On top of the the current status, rename org.apache.flink.api.connector.sink2.InitContext to CommonInitContext (should >probably be package private). >2. Change the name WriterInitContext back to InitContext, and revert the deprecation. We can change the parameter name to writerContext if we want to. >Admittedly, this does not have full symmetric naming of the InitContexts - we will have CommonInitContext / InitContext / CommitterInitContext instead of InitContext / WriterInitContext / CommitterInitContext. However, the naming seems clear without much confusion. Personally, I can live with that, treating the class InitContext as a non-ideal legacy class name without much material harm. The alternative approach has less changes to the existing interface. The only caveat is we have to keep the legacy name of `InitContext`. However, given that we are going to replace `StatefulSink` and `TwoPhaseCommttingSink` with mixin interfaces, the naming issue is just temporary. We can use the new name for InitContexts in the new mixin interfaces. From this standpoint, personally I prefer the alternative approach as it introduces less changes to the existing interfaces that we are about to deprecate. 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
