Hi Sebastian, Thanks for picking this up.
> 5. There is NO configuration option to enable watermark alignment of splits or disable pause/resume capabilities. Isn't this contradicting what we actually agreed on? > we are planning to have a configuration based way to revert to the previous behavior I think what we agreed in the last couple of emails was to add a configuration toggle, that would allow Flink 1.15 users, that are using watermark alignment with multiple splits per source operator, to continue using it with the old 1.15 semantic, even if their source doesn't support pausing/resuming splits. It seems to me like the current FLIP and implementation proposal would always throw an exception in that case? Best, Piotrek wt., 12 lip 2022 o 10:18 Sebastian Mattheis <sebast...@ververica.com> napisał(a): > Hi all, > > I have updated FLIP-217 [1] to the proposed specification and adapted the > current implementation [2] respectively. > > This means both, FLIP and implementation, are ready for review from my > side. (I would revise commit history and messages for the final PR but left > it as is for now and the records of this discussion.) > > 1. Please review the updated version of FLIP-217 [1]. If there are no > further concerns, I would initiate the voting. > (2. If you want to speed up things, please also have a look into the > updated implementation [2].) > > Please consider the following updated specification in the current status > of FLIP-217 where the essence is as follows: > > 1. A method pauseOrResumeSplits is added to SourceReader with default > implementation that throws UnsupportedOperationException. > 2. method pauseOrResumeSplits is added to SplitReader with default > implementation that throws UnsupportedOperationException. > 3. SourceOperator initiates split alignment only if more than one split is > assigned to the source (and, of course, only if withSplitAlignment is used). > 4. There is NO "supportsPauseOrResumeSplits" method at any place (to > indicate if the implementation supports pause/resume capabilities). > 5. There is NO configuration option to enable watermark alignment of > splits or disable pause/resume capabilities. > > *Note:* If the SourceReader or some SplitReader do not override > pauseOrResumeSplits but it is required in the application, an exception is > thrown at runtime when an split alignment attempt is executed (not during > startup or any time earlier). > > Also, I have revised the compatibility/migration section to describe a bit > of a rationale for the default implementation with exception throwing > behavior. > > Regards, > Sebastian > > [1] > https://cwiki.apache.org/confluence/display/FLINK/FLIP-217+Support+watermark+alignment+of+source+splits > [2] https://github.com/smattheis/flink/tree/flip-217-split-wm-alignment > > On Mon, Jul 4, 2022 at 3:59 AM Thomas Weise <t...@apache.org> wrote: > >> Hi, >> >> Thank you Becket and Piotr for ironing out the "case 2" behavior. >> Strictly speaking we are introducing a regression by allowing an >> exception to bubble up that did not exist in the previous release, >> regardless how suboptimal the behavior may be. However, given that the >> feature is still experimental and we are planning to have a >> configuration based way to revert to the previous behavior, I think >> this is a good solution. >> >> +1 from my side >> >> Thomas >> >> On Wed, Jun 29, 2022 at 2:43 PM Piotr Nowojski <pnowoj...@apache.org> >> wrote: >> > >> > +1 :) >> > >> > śr., 29 cze 2022 o 17:23 Becket Qin <becket....@gmail.com> napisał(a): >> > >> > > Thanks for the explanation, Piotr. >> > > >> > > So it looks like we have a conclusion here. >> > > >> > > 1. Regarding the supportsPausingSplits() method, I feel it brings more >> > > confusion while the benefit is marginal, so I prefer not having that >> if >> > > possible. It would be good to also hear @Thomas Weise <t...@apache.org >> >'s >> > > opinion as he mentioned some concern earlier. >> > > 2. Let's add the feature knob then. In the future we can simply >> ignore the >> > > configuration when deprecating it. >> > > >> > > Thanks, >> > > >> > > Jiangjie (Becket) Qin >> > > >> > > On Wed, Jun 29, 2022 at 10:19 PM Piotr Nowojski <pnowoj...@apache.org >> > >> > > wrote: >> > > >> > > > Hi, >> > > > >> > > > I mean I'm fine with throwing an exception by default in Flink 1.16 >> in >> > > the >> > > > "Case 2", but I think we need to provide a way to workaround it for >> > > example >> > > > via a feature toggle, if it's an easy thing to do. And it seems to >> be a >> > > > simple thing. >> > > > >> > > > However this is orthogonal to the `supportsPausingSplits()` issue. I >> > > don't >> > > > have a big preference whether >> > > > a) the exception should originate on JM, using `default boolean >> > > > supportsPausingSplits() { return false; }` (as currently proposed >> in the >> > > > FLIP), >> > > > b) or on the TM from `pauseOrResumeSplits()` throwing >> > > > `UnsupportedOperationException` as you are proposing. >> > > > >> > > > a) fails earlier, so it's more user friendly from this perspective, >> but >> > > it >> > > > provides more possibilities for bugs/inconsistencies for connector >> > > > developers, since `supportsPausingSplits()` would have to be kept >> in sync >> > > > with `pauseOrResumeSplits()`. >> > > > >> > > > Best, >> > > > Piotrek >> > > > >> > > > śr., 29 cze 2022 o 15:27 Becket Qin <becket....@gmail.com> >> napisał(a): >> > > > >> > > > > Hi Piotr, >> > > > > >> > > > > Just to make sure we are on the same page. There are two cases >> for the >> > > > > existing FLIP-182 users: >> > > > > >> > > > > Case 1: Each source reader only has one split assigned. This is >> the >> > > > > targeted case for FLIP-182. >> > > > > Case 2: Each source reader has multiple splits assigned. This is >> the >> > > > flaky >> > > > > case that may or may not work. >> > > > > >> > > > > With solution 1, the users of case 1 won't be impacted. The users >> in >> > > > case 2 >> > > > > will receive an exception which they won't get at the moment. >> > > > > >> > > > > Do you mean we should not throw an exception in case 2? >> Personally I >> > > feel >> > > > > that is OK and could have been done in FLIP-182 itself because >> it's >> > > not a >> > > > > designed use case. As a user I may see a big variation of the job >> state >> > > > > sizes from time to time and I am not able to rely on this feature >> to >> > > plan >> > > > > my resources and uphold the SLA. >> > > > > >> > > > > That said, if you have a strong opinion on this, I am fine with >> having >> > > > the >> > > > > configuration like "allow.coarse-grained.watermark.alignment" >> with the >> > > > > default value set to false, given that a configuration is much >> easier >> > > to >> > > > > deprecate than a method. >> > > > > >> > > > > Thanks, >> > > > > >> > > > > Jiangjie (Becket) Qin >> > > > > >> > > > > >> >
