The PR is merged, so everything should be back to normal. Thanks everyone involved!
Also based on the discussion here, I have updated the FLIP-372 [1]. I tried to incorporate every comment, but in some cases they were contradictory, so please share your opinion on the corresponding thread [2]. Thanks again, Peter [1] - https://cwiki.apache.org/confluence/display/FLINK/FLIP-372%3A+Allow+TwoPhaseCommittingSink+WithPreCommitTopology+to+alter+the+type+of+the+Committable [2] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd Leonard Xu <xbjt...@gmail.com> ezt írta (időpont: 2023. dec. 12., K, 7:33): > Thanks Peter and Marton for the quick response and fix, I’ve left +1 for > this PR. > > I’d like to take a look at followup FLIP-372 as well. > > Best, > Leonard > > > > 2023年12月12日 上午7:39，Becket Qin <becket....@gmail.com> 写道： > > > > Hi Peter, > > > > Thanks for updating the patch. The latest patch looks good to me. I've > +1ed > > on the PR. > > > > Cheers, > > > > Jiangjie (Becket) Qin > > > > On Mon, Dec 11, 2023 at 9:21 PM Péter Váry <peter.vary.apa...@gmail.com> > > wrote: > > > >> Thanks everyone for the lively discussion! > >> > >> The PR is available which strictly adheres the accepted changes from > >> FLIP-371. Thanks Gyula and Marton for the review. Becket, if you have > any > >> questions left, please let me know, so I can fix and we can merge the > >> changes. > >> > >> I would like to invite everyone involved here to take a look at FLIP-372 > >> [1], and the related mailing thread [2]. The discussion there is also at > >> the stage where we are debating the merits of migrating to a mixin based > >> Sink API. So if you are interested, please join us there. > >> > >> Thanks, > >> Peter > >> > >> [1] - > >> > >> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-372%3A+Allow+TwoPhaseCommittingSink+WithPreCommitTopology+to+alter+the+type+of+the+Committable > >> [2] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd > >> > >> > >> On Tue, Dec 5, 2023, 18:05 Márton Balassi <balassi.mar...@gmail.com> > >> wrote: > >> > >>> Thanks, Peter. Given the discussion I also agree that the consensus is > to > >>> move towards the mixin interface approach (and accept its disadvantages > >>> given its advantages). > >>> > >>> +1 for the general direction of your proposed code change in > >>> https://github.com/apache/flink/pull/23876. > >>> > >>> On Tue, Dec 5, 2023 at 3:44 PM Péter Váry <peter.vary.apa...@gmail.com > > > >>> wrote: > >>> > >>>> It seems to me we have a consensus to move forward with the mixin > >>> approach. > >>>> I hope that everyone is aware that with the mixin interfaces we lose > >> the > >>>> opportunity of the strong type checks. This will be especially painful > >>> for > >>>> generic types, where we will not have a way to ensure that the generic > >>>> types are correctly synchronized between the different interfaces, > even > >>> on > >>>> DAG creation time. > >>>> > >>>> Even with this drawback, I like this approach too, so +1 from my side. > >>>> > >>>> As a first step in the direction of the mixin approach, we can remove > >> the > >>>> specific implementations of the `createWriter` methods from the > >>>> `StatefulSink` and the `TwoPhaseCommitingSink` interfaces (and replace > >>> them > >>>> with an instanceof check where needed). > >>>> - This would remove the diamond inheritance - enable us to create > >> default > >>>> methods for backward compatibility. > >>>> - This would not break the API, as the same method with wider return > >>> value > >>>> will be inherited from the `Sink` interface. > >>>> > >>>> Since, it might be easier to understand the proposed changes, I have > >>>> created a new PR: https://github.com/apache/flink/pull/23876 > >>>> The PR has 2 commits: > >>>> - Reverting the previous change - non-clean, since there were some > >>>> additional fixes on the tests - > >>>> > >>>> > >>> > >> > https://github.com/apache/flink/pull/23876/commits/c7625d5fa62a6e9a182f39f53fb7e5626105f3b0 > >>>> - The new change with mixin approach, and deprecation - > >>>> > >>>> > >>> > >> > https://github.com/apache/flink/pull/23876/commits/99ec936966af527598ca49712c1263bc4aa03c15 > >>>> > >>>> Thanks, > >>>> Peter > >>>> > >>>> weijie guo <guoweijieres...@gmail.com> ezt írta (időpont: 2023. dec. > >> 5., > >>>> K, > >>>> 8:01): > >>>> > >>>>> Thanks Martijn for driving this! > >>>>> > >>>>> I'm +1 to reverting the breaking change. > >>>>> > >>>>>> For new functionality or changes we can make easily, we should > >> switch > >>>> to > >>>>> the decorative/mixin interface approach used successfully in the > >> source > >>>> and > >>>>> table interfaces. > >>>>> > >>>>> I like the way of switching to mixin interface. > >>>>> > >>>>> Best regards, > >>>>> > >>>>> Weijie > >>>>> > >>>>> > >>>>> Becket Qin <becket....@gmail.com> 于2023年12月5日周二 14:50写道： > >>>>> > >>>>>> I am with Gyula about fixing the current SinkV2 API. > >>>>>> > >>>>>> A SinkV3 seems not necessary because we are not changing the > >>>> fundamental > >>>>>> design of the API. Hopefully we can modify the interface structure > >> a > >>>>> little > >>>>>> bit to make it similar to the Source while still keep the backwards > >>>>>> compatibility. > >>>>>> For example, one approach is: > >>>>>> > >>>>>> - Add snapshotState(int checkpointId) and precommit() methods to > >> the > >>>>>> SinkWriter with default implementation doing nothing. Deprecate > >>>>>> StatefulSinkWriter and PrecommittingSinkWriter. > >>>>>> - Add two mixin interfaces of SupportsStatefulWrite and > >>>>>> SupportsTwoPhaseCommit. Deprecate the StatefulSink and > >>>>>> TwoPhaseCommittingSink. > >>>>>> > >>>>>> Thanks, > >>>>>> > >>>>>> Jiangjie (Becket) Qin > >>>>>> > >>>>>> On Mon, Dec 4, 2023 at 7:25 PM Gyula Fóra <gyula.f...@gmail.com> > >>>> wrote: > >>>>>> > >>>>>>> Hi All! > >>>>>>> > >>>>>>> Based on the discussion above, I feel that the most reasonable > >>>> approach > >>>>>>> from both developers and users perspective at this point is what > >>>> Becket > >>>>>>> lists as Option 1: > >>>>>>> > >>>>>>> Revert the naming change to the backward compatible version and > >>>> accept > >>>>>> that > >>>>>>> the names are not perfect (treat it as legacy). > >>>>>>> > >>>>>>> On a different note, I agree that the current sink v2 interface > >> is > >>>> very > >>>>>>> difficult to evolve and structuring the interfaces the way they > >> are > >>>> now > >>>>>> is > >>>>>>> not a good design in the long run. > >>>>>>> For new functionality or changes we can make easily, we should > >>> switch > >>>>> to > >>>>>>> the decorative/mixin interface approach used successfully in the > >>>> source > >>>>>> and > >>>>>>> table interfaces. Let's try to do this as much as possible within > >>> the > >>>>> v2 > >>>>>>> and compatibility boundaries and we should only introduce a v3 if > >>> we > >>>>>> really > >>>>>>> must. > >>>>>>> > >>>>>>> So from my side, +1 to reverting the naming to keep backward > >>>>>> compatibility. > >>>>>>> > >>>>>>> Cheers, > >>>>>>> Gyula > >>>>>>> > >>>>>>> > >>>>>>> On Fri, Dec 1, 2023 at 10:43 AM Péter Váry < > >>>>> peter.vary.apa...@gmail.com> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> Thanks Becket for your reply! > >>>>>>>> > >>>>>>>> *On Option 1:* > >>>>>>>> - I personally consider API inconsistencies more important, > >> since > >>>>> they > >>>>>>> will > >>>>>>>> remain with us "forever", but this is up to the community. I > >> can > >>>>>>> implement > >>>>>>>> whichever solution we decide upon. > >>>>>>>> > >>>>>>>> *Option 2:* > >>>>>>>> - I don't think this specific issue merits a rewrite, but if we > >>>>> decide > >>>>>> to > >>>>>>>> change our approach, then it's a different story. > >>>>>>>> > >>>>>>>> *Evolvability:* > >>>>>>>> This discussion reminds me of a similar discussion on FLIP-372 > >>> [1], > >>>>>> where > >>>>>>>> we are trying to decide if we should use mixin interfaces, or > >> use > >>>>>>> interface > >>>>>>>> inheritance. > >>>>>>>> With the mixin approach, we have a more flexible interface, but > >>> we > >>>>>> can't > >>>>>>>> check the generic types of the interfaces/classes on compile > >>> time, > >>>> or > >>>>>>> even > >>>>>>>> when we create the DAG. The first issue happens when we call > >> the > >>>>> method > >>>>>>> and > >>>>>>>> fail. > >>>>>>>> The issue here is similar: > >>>>>>>> - *StatefulSink* needs a writer with a method to > >>> `*snapshotState*` > >>>>>>>> - *TwoPhaseCommittingSink* needs a writer with > >> `*prepareCommit*` > >>>>>>>> - If there is a Sink which is stateful and needs to commit, > >> then > >>> it > >>>>>> needs > >>>>>>>> both of these methods. > >>>>>>>> > >>>>>>>> If we use the mixin solution here, we lose the possibility to > >>> check > >>>>> the > >>>>>>>> types in compile time. We could do the type check in runtime > >>> using > >>>> ` > >>>>>>>> *instanceof*`, so we are better off than with the FLIP-372 > >>> example > >>>>>> above, > >>>>>>>> but still lose any important possibility. I personally prefer > >> the > >>>>> mixin > >>>>>>>> approach, but that would mean we rewrite the Sink API again - > >>>> likely > >>>>> a > >>>>>>>> SinkV3. Are we ready to move down that path? > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> Peter > >>>>>>>> > >>>>>>>> [1] - > >>>>> https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd > >>>>>>>> > >>>>>>>> On Thu, Nov 30, 2023, 14:53 Becket Qin <becket....@gmail.com> > >>>> wrote: > >>>>>>>> > >>>>>>>>> Hi folks, > >>>>>>>>> > >>>>>>>>> Sorry for replying late on the thread. > >>>>>>>>> > >>>>>>>>> For this particular FLIP, I see two solutions: > >>>>>>>>> > >>>>>>>>> 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. > >>>>>>>>> > >>>>>>>>> Option 2: > >>>>>>>>> Theoretically speaking, if we really want to reach the > >> perfect > >>>>> state > >>>>>>>> while > >>>>>>>>> being backwards compatible, we can create a brand new set of > >>> Sink > >>>>>>>>> interfaces and deprecate the old ones. But I feel this is an > >>>>> overkill > >>>>>>>> here. > >>>>>>>>> > >>>>>>>>> The solution to this particular issue aside, the evolvability > >>> of > >>>>> the > >>>>>>>>> current interface hierarchy seems a more fundamental issue > >> and > >>>>>> worries > >>>>>>> me > >>>>>>>>> more. I haven't completely thought it through, but there are > >>> two > >>>>>>>> noticeable > >>>>>>>>> differences between the interface design principles between > >>>> Source > >>>>>> and > >>>>>>>>> Sink. > >>>>>>>>> 1. Source uses decorative interfaces. For example, we have a > >>>>>>>>> SupportsFilterPushdown interface, instead of a subclass of > >>>>>>>>> FilterableSource. This seems provides better flexibility. > >>>>>>>>> 2. Source tends to have a more coarse-grained interface. For > >>>>> example, > >>>>>>>>> SourceReader always has the methods of snapshotState(), > >>>>>>>>> notifyCheckpointComplete(). Even if they may not be always > >>>>> required, > >>>>>> we > >>>>>>>> do > >>>>>>>>> not separate them into different interfaces. > >>>>>>>>> My hunch is that if we follow similar approach as Source, the > >>>>>>>> evolvability > >>>>>>>>> might be better. If we want to do this, we'd better to do it > >>>> before > >>>>>>> 2.0. > >>>>>>>>> What do you think? > >>>>>>>>> > >>>>>>>>> Process wise, > >>>>>>>>> - I agree that if there is a change to the passed FLIP during > >>>>>>>>> implementation, it should be brought back to the mailing > >> list. > >>>>>>>>> - There might be value for the connector nightly build to > >>> depend > >>>> on > >>>>>> the > >>>>>>>>> latest snapshot of the same Flink major version. It helps > >>>> catching > >>>>>>>>> unexpected breaking changes sooner. > >>>>>>>>> - I'll update the website to reflect the latest API stability > >>>>> policy. > >>>>>>>>> Apologies for the confusion caused by the stale doc. > >>>>>>>>> > >>>>>>>>> Thanks, > >>>>>>>>> > >>>>>>>>> Jiangjie (Becket) Qin > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On Wed, Nov 29, 2023 at 10:55 PM Márton Balassi < > >>>>>>>> balassi.mar...@gmail.com> > >>>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> Thanks, Martijn and Peter. > >>>>>>>>>> > >>>>>>>>>> In terms of the concrete issue: > >>>>>>>>>> > >>>>>>>>>> - I am following up with the author of FLIP-321 [1] > >>> (Becket) > >>>>> to > >>>>>>>> update > >>>>>>>>>> the docs [2] to reflect the right state. > >>>>>>>>>> - I see two reasonable approaches in terms of proceeding > >>>> with > >>>>>> the > >>>>>>>>>> specific changeset: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> 1. We allow the exception from FLIP-321 for this change > >>> and > >>>>> let > >>>>>>> the > >>>>>>>>>> PublicEvolving API change happen between Flink 1.18 > >> and > >>>>> 1.19, > >>>>>>>> which > >>>>>>>>>> is > >>>>>>>>>> consistent with current state of the relevant > >>>>> documentation. > >>>>>>> [2] > >>>>>>>>>> We commit > >>>>>>>>>> to helping the connector repos make the necessary > >> (one > >>>>> liner) > >>>>>>>>>> changes. > >>>>>>>>>> 2. We revert back to the original implementation plan > >>> as > >>>>>>>> explicitly > >>>>>>>>>> voted on in FLIP-371 [3]. That has no API breaking > >>>> changes. > >>>>>>>>>> However we end > >>>>>>>>>> up with an inconsistently named API with duplicated > >>>>> internal > >>>>>>>>>> methods. Peter > >>>>>>>>>> has also discovered additional bad patterns during > >> his > >>>> work > >>>>>> in > >>>>>>>>>> FLIP-372 > >>>>>>>>>> [3], the total of these changes could be handled in a > >>>>>> separate > >>>>>>>> FLIP > >>>>>>>>>> that > >>>>>>>>>> would do multiple PublicEvolving breaking changes to > >>>> clean > >>>>> up > >>>>>>> the > >>>>>>>>>> API. > >>>>>>>>>> > >>>>>>>>>> In terms of the general issues: > >>>>>>>>>> > >>>>>>>>>> - I agree that if a PR review of an accepted FLIP newly > >>>>>>> introduces a > >>>>>>>>>> breaking API change that warrants an update to the > >> mailing > >>>>> list > >>>>>>>>>> discussion > >>>>>>>>>> and possibly even a new vote. > >>>>>>>>>> - I agree with the general sentiment of FLIP-321 to > >>> provide > >>>>>>> stronger > >>>>>>>>> API > >>>>>>>>>> guarantees with the minor note that if we have changes > >> in > >>>> mind > >>>>>> we > >>>>>>>>> should > >>>>>>>>>> prioritize them now such that they can be validated by > >>> Flink > >>>>>> 2.0. > >>>>>>>>>> - I agree that ideally the connector repos should build > >>>>> against > >>>>>>> the > >>>>>>>>>> latest release and not the master branch. > >>>>>>>>>> > >>>>>>>>>> [1] > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-321%3A+Introduce+an+API+deprecation+process > >>>>>>>>>> [2] > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://nightlies.apache.org/flink/flink-docs-master/docs/ops/upgrading/#api-compatibility-guarantees > >>>>>>>>>> [3] > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-371%3A+Provide+initialization+context+for+Committer+creation+in+TwoPhaseCommittingSink > >>>>>>>>>> [4] > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-372%3A+Allow+TwoPhaseCommittingSink+WithPreCommitTopology+to+alter+the+type+of+the+Committable > >>>>>>>>>> > >>>>>>>>>> Best, > >>>>>>>>>> Marton > >>>>>>>>>> > >>>>>>>>>> On Mon, Nov 27, 2023 at 7:23 PM Péter Váry < > >>>>>>>> peter.vary.apa...@gmail.com> > >>>>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>>> I think we should try to separate the discussion in a few > >>>>>> different > >>>>>>>>>> topics: > >>>>>>>>>>> > >>>>>>>>>>> - Concrete issue > >>>>>>>>>>> - How to solve this problem in 1.19 and wrt the > >>>> affected > >>>>>>>>>> createWriter > >>>>>>>>>>> interface > >>>>>>>>>>> - Update the documentation [1], so FLIP-321 is > >>> visible > >>>>> for > >>>>>>>> every > >>>>>>>>>>> contributor > >>>>>>>>>>> - Generic issue > >>>>>>>>>>> - API stability > >>>>>>>>>>> - Connector dependencies > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> *CreateWriter interface* > >>>>>>>>>>> The change on the createWriter is not strictly required > >> for > >>>> the > >>>>>>>>>>> functionality defined by the requirements on the FLIP. > >>>>>>>>>>> If the only goal is only to have a backward compatible > >> API, > >>>> we > >>>>>> can > >>>>>>>>> simply > >>>>>>>>>>> create a separate `*CommitterInitContext*` object and do > >>> not > >>>>>> touch > >>>>>>>> the > >>>>>>>>>>> writer `*InitContext*`, like it was done in the original > >> PR > >>>>> [2]. > >>>>>>>>>>> The issue is that this would result in an implementation > >>>> which > >>>>>> has > >>>>>>>>>>> duplicated methods/implementations (internal issue only), > >>> and > >>>>> has > >>>>>>>>>>> inconsistent naming (issue for external users). > >>>>>>>>>>> > >>>>>>>>>>> If we want to create an API which is consistent (and I > >>> agree > >>>>> with > >>>>>>> the > >>>>>>>>>>> reviewer's comments), then we need to rename the > >> parameter > >>>>> type ( > >>>>>>>>>>> *WriterInitContext*) for the createWriter method. > >>>>>>>>>>> I have tried to keep the backward compatibility with > >>>> creating a > >>>>>> new > >>>>>>>>>> method > >>>>>>>>>>> and providing a default implementation for this new > >> method > >>>>> which > >>>>>>>> would > >>>>>>>>>> call > >>>>>>>>>>> the original method after converting the > >> WriterInitContext > >>> to > >>>>>>>>>> InitContext. > >>>>>>>>>>> > >>>>>>>>>>> This is failed because the following details: > >>>>>>>>>>> > >>>>>>>>>>> - *org.apache.flink.api.connector.sink2.Sink* defines > >>>>>>>>>>> `*SinkWriter<InputT> > >>>>>>>>>>> createWriter(InitContext context)`* > >>>>>>>>>>> - *org.apache.flink.api.connector.sink2.StatefulSink* > >>>>> narrows > >>>>>> it > >>>>>>>>>>> down to *`StatefulSinkWriter<InputT, > >>>>>>>>>>> WriterStateT> createWriter(InitContext context)`* > >>>>>>>>>>> - > >>>>>> *org.apache.flink.api.connector.sink2.TwoPhaseCommittingSink* > >>>>>>>>>> narrows > >>>>>>>>>>> it down to *`PrecommittingSinkWriter<InputT, CommT> > >>>>>>>>>>> createWriter(WriterInitContext context)`* > >>>>>>>>>>> - > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > *org.apache.flink.streaming.runtime.operators.sink.TestSinkV2.TestStatefulSinkV2* > >>>>>>>>>>> implements *StatefulSink* and *TwoPhaseCommittingSink* > >>> too > >>>>>>>>>>> > >>>>>>>>>>> *TestStatefulSinkV2* is a good example where we can not > >>>> achieve > >>>>>>>>> backward > >>>>>>>>>>> compatibility, since the the compiler will fail with > >>>> unrelated > >>>>>>>> default > >>>>>>>>>>> methods [3] > >>>>>>>>>>> > >>>>>>>>>>> I am open for any suggestions how to move to the new API, > >>> and > >>>>>> keep > >>>>>>>> the > >>>>>>>>>>> backward compatibility. If we do not find a way to keep > >>>>> backward > >>>>>>>>>>> compatibility, and we decide that we would like to honour > >>>>>> FLIP-321, > >>>>>>>>> then > >>>>>>>>>> we > >>>>>>>>>>> can reverting to the original solution and keep only the > >>>>> changes > >>>>>>> for > >>>>>>>>> the > >>>>>>>>>> ` > >>>>>>>>>>> *createCommitter*` method. > >>>>>>>>>>> > >>>>>>>>>>> *Update the documentation* > >>>>>>>>>>> I have not found only one place in the docs [1], where we > >>>> talk > >>>>>>> about > >>>>>>>>> the > >>>>>>>>>>> compatibility guarantees. > >>>>>>>>>>> Based FLIP-321 and the result of the discussion here, we > >>>> should > >>>>>>>> update > >>>>>>>>>> this > >>>>>>>>>>> page. > >>>>>>>>>>> > >>>>>>>>>>> *API stability* > >>>>>>>>>>> I agree with the general sentiment of FLIP-321 to keep > >> the > >>>>>> changes > >>>>>>>>>> backward > >>>>>>>>>>> compatible as much as possible. But the issue above > >>>> highlights > >>>>>> that > >>>>>>>>> there > >>>>>>>>>>> could be situations where it is not possible to achieve > >>>>> backward > >>>>>>>>>>> compatibility. Probably we should provide exceptions to > >>>> handle > >>>>>> this > >>>>>>>>> kind > >>>>>>>>>> of > >>>>>>>>>>> situations - minimally for PublicEvolving interfaces. > >> After > >>>> we > >>>>>>> agree > >>>>>>>> on > >>>>>>>>>>> long term goals - allowing exceptions or to be more > >> lenient > >>>> on > >>>>>>>> backward > >>>>>>>>>>> compatibility guarantees, or sticking to FLIP-321 by the > >>>>> letter - > >>>>>>> we > >>>>>>>>>> could > >>>>>>>>>>> discuss how to apply it to the current situation. > >>>>>>>>>>> > >>>>>>>>>>> *Connector dependencies* > >>>>>>>>>>> I think it is generally a good practice to depend on the > >>>> stable > >>>>>>>> version > >>>>>>>>>> of > >>>>>>>>>>> Flink (or any other downstream project). This is how we > >> do > >>> it > >>>>> in > >>>>>>>>> Iceberg, > >>>>>>>>>>> and how it was implemented in the Kafka connector as > >> well. > >>>> This > >>>>>>> would > >>>>>>>>>>> result in more stable connector builds. The only issue I > >>> see, > >>>>>> that > >>>>>>>> the > >>>>>>>>>>> situations like this would take longer to surface, but I > >>>> fully > >>>>>>> expect > >>>>>>>>> us > >>>>>>>>>> to > >>>>>>>>>>> get better at compatibility after we wetted the process. > >>>>>>>>>>> > >>>>>>>>>>> [1] - > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://nightlies.apache.org/flink/flink-docs-master/docs/ops/upgrading/#api-compatibility-guarantees > >>>>>>>>>>> [2] - > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://github.com/apache/flink/pull/23555/commits/2b9adeb20e55c33a623115efa97d3149c11e9ca4 > >>>>>>>>>>> [3] - > >>>>>>>>> > >>>> https://github.com/apache/flink/pull/23555#discussion_r1371740397 > >>>>>>>>>>> > >>>>>>>>>>> Martijn Visser <martijnvis...@apache.org> ezt írta > >>> (időpont: > >>>>>> 2023. > >>>>>>>>> nov. > >>>>>>>>>>> 27., H, 11:21): > >>>>>>>>>>> > >>>>>>>>>>>> Hi all, > >>>>>>>>>>>> > >>>>>>>>>>>> I'm opening this discussion thread to bring a > >> discussion > >>>>> that's > >>>>>>>>>>>> happening on a completed Jira ticket back to the > >> mailing > >>>> list > >>>>>> [1] > >>>>>>>>>>>> > >>>>>>>>>>>> In summary: > >>>>>>>>>>>> > >>>>>>>>>>>> * There was a discussion and a vote on FLIP-371 [2] > >>>>>>>>>>>> * During implementation, it was determined that > >> there's a > >>>>>> diamond > >>>>>>>>>>>> inheritance problem on the Sink.createWriter method, > >>>> making a > >>>>>>>>>>>> backwards compatible change hard/impossible (I think > >> this > >>>> is > >>>>>>> where > >>>>>>>>> the > >>>>>>>>>>>> main discussion point actually is) [3] > >>>>>>>>>>>> * The PR was merged, causing a backwards incompatible > >>>> change > >>>>>>>> without > >>>>>>>>> a > >>>>>>>>>>>> discussion on the Dev mailing list > >>>>>>>>>>>> > >>>>>>>>>>>> I think that in hindsight, even though there was a FLIP > >>> on > >>>>> this > >>>>>>>>> topic, > >>>>>>>>>>>> the finding of the diamond inheritance issue should > >> have > >>>> been > >>>>>>>> brought > >>>>>>>>>>>> back to the Dev mailing list in order to agree on how > >> to > >>>>>> resolve > >>>>>>>> it. > >>>>>>>>>>>> Since 1.19 is still under way, we still have time to > >> fix > >>>>> this. > >>>>>>>>>>>> > >>>>>>>>>>>> I think there's two things we can improve: > >>>>>>>>>>>> > >>>>>>>>>>>> 1) Next time during implementation of a FLIP/PR which > >>>>> involves > >>>>>> a > >>>>>>>>>>>> non-backward compatible change of an API that wasn't > >>>>> accounted > >>>>>>> for, > >>>>>>>>>>>> the discussion should be brought back to the Dev > >> mailing > >>>>> list. > >>>>>> I > >>>>>>>>> think > >>>>>>>>>>>> we can just add that to the FLIP bylaws. > >>>>>>>>>>>> 2) How do we actually resolve the problem: is there > >>> anyone > >>>>> who > >>>>>>> has > >>>>>>>> an > >>>>>>>>>>>> idea on how we could introduce the proposed change > >> while > >>>>>>>> maintaining > >>>>>>>>>>>> backwards compatibility, or do we agree that while this > >>> is > >>>> an > >>>>>> non > >>>>>>>>>>>> desired situation, there is no better alternative > >>>>>> unfortunately? > >>>>>>>>>>>> > >>>>>>>>>>>> Best regards, > >>>>>>>>>>>> > >>>>>>>>>>>> Martijn > >>>>>>>>>>>> > >>>>>>>>>>>> [1] https://issues.apache.org/jira/browse/FLINK-25857 > >>>>>>>>>>>> [2] > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-371%3A+Provide+initialization+context+for+Committer+creation+in+TwoPhaseCommittingSink > >>>>>>>>>>>> [3] > >>>>>>>>> > >>>> https://github.com/apache/flink/pull/23555#discussion_r1371740397 > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > >
