Thanks Timo and Jim! Added a few sentences to the FLIP to cover your points. -Alexey
On Mon, Jun 10, 2024 at 11:23 PM Timo Walther <twal...@apache.org> wrote: > Hi Alexey, > > thanks for proposing this FLIP. It is a nice continuation of the vision > we had for CompiledPlan when writing and implementing FLIP-190. The > whole stack is prepared for serializing BatchExecNodes as well so it > shouldn't be too hard to make this a reality. > > > I think the FLIP should be clear on the backwards support strategy > > here. The strategy for streaming is "forever". This may be the most > > interesting part of the FLIP to discuss. > > I agree with Jim. We shouldn't put too much burden on us (the Flink > community). BatchExecNodes can evolve quicker than StreamExecNodes as > the state component isn't an issue. Backwards compatibility of 2-3 Flink > versions and at least 1 year of time should be enough for batch > infrastructure to update. Of course we should avoid breaking changes > whenever possible. This should be written down in the FLIP. > > Regards, > Timo > > > > > On 07.06.24 23:10, Jim Hughes wrote: > > Hi Alexey, > > > > Responses inline below: > > > > On Mon, May 13, 2024 at 7:18 PM Alexey Leonov-Vendrovskiy < > > vendrov...@gmail.com> wrote: > > > >> Thanks Jim. > >> > >>> 1. For the testing, I'd call the tests "execution" tests rather than > >>> "restore" tests. For streaming execution, restore tests have the > >> compiled > >>> plan and intermediate state; the tests verify that those can work > >> together > >>> and continue processing. > >> > >> Agree that we don't need to store and restore the intermediate state. So > >> the most critical part is that the CompiledPlan for batch can be > executed. > >> > > > > On the FLIP, can you be more specific about what we are checking during > > execution? I'd suggest that `executeSql(_)` and > > `executePlan(compilePlanSql(_))` should be compared. > > > > > >> 2. The FLIP implicitly suggests "completeness tests" (to use FLIP-190's > >>> words). Do we need "change detection tests"? I'm a little unsure if > >> that > >>> is presently happening in an automatic way for streaming operators. > >> > >> > >> We might need to elaborate more on this, but the idea is that we > need to > >> make sure that compiled plans created by an older version of SQL Planner > >> are executable on newer runtimes. > >> > >> 3. Can we remove old versions of batch operators eventually? Or do we > >>> need to keep them forever like we would for streaming operators? > >>> > >> > >> We could have deprecation paths for old operator nodes in some cases. > It is > >> a matter of the time window: what could be practical the "time distance" > >> between query planner and flink runtime against which the query query > can > >> be resubmitted. > >> Note, here we don't have continuous queries, so there is always an > option > >> to "re-plan" the original SQL query text into a newer version of the > >> CompiledPlan. > >> With this in mind, a time window of 1yr+ would allow deprecation of > older > >> batch exec nodes, though I don't see this as a frequent event. > >> > > > > As I read the JavaDocs for `TableEnvironment.loadPlan`, it looks like the > > compiled plan ought to be sufficient to run a job at a later time. > > > > I think the FLIP should be clear on the backwards support strategy here. > > The strategy for streaming is "forever". This may be the most > interesting > > part of the FLIP to discuss. > > > > Can you let us know when you've updated the FLIP? > > > > Cheers, > > > > Jim > > > > > >> -Alexey > >> > >> > >> > >> On Mon, May 13, 2024 at 1:52 PM Jim Hughes <jhug...@confluent.io.invalid > > > >> wrote: > >> > >>> Hi Alexey, > >>> > >>> After some thought, I have a question about deprecations: > >>> > >>> 3. Can we remove old versions of batch operators eventually? Or do we > >>> need to keep them forever like we would for streaming operators? > >>> > >>> Cheers, > >>> > >>> Jim > >>> > >>> On Thu, May 9, 2024 at 11:29 AM Jim Hughes <jhug...@confluent.io> > wrote: > >>> > >>>> Hi Alexey, > >>>> > >>>> Overall, the FLIP looks good and makes sense to me. > >>>> > >>>> 1. For the testing, I'd call the tests "execution" tests rather than > >>>> "restore" tests. For streaming execution, restore tests have the > >>> compiled > >>>> plan and intermediate state; the tests verify that those can work > >>> together > >>>> and continue processing. > >>>> > >>>> For batch execution, I think we just want that all existing compiled > >>> plans > >>>> can be executed in future versions. > >>>> > >>>> 2. The FLIP implicitly suggests "completeness tests" (to use > FLIP-190's > >>>> words). Do we need "change detection tests"? I'm a little unsure if > >>> that > >>>> is presently happening in an automatic way for streaming operators. > >>>> > >>>> In RestoreTestBase, generateTestSetupFiles is disabled and has to be > >> run > >>>> manually when tests are being written. > >>>> > >>>> Cheers, > >>>> > >>>> Jim > >>>> > >>>> On Tue, May 7, 2024 at 5:11 AM Paul Lam <paullin3...@gmail.com> > wrote: > >>>> > >>>>> Hi Alexey, > >>>>> > >>>>> Thanks a lot for bringing up the discussion. I’m big +1 for the FLIP. > >>>>> > >>>>> I suppose the goal doesn’t involve the interchangeability of json > >> plans > >>>>> between batch mode and streaming mode, right? > >>>>> In other words, a json plan compiled in a batch program can’t be run > >> in > >>>>> streaming mode without a migration (which is not yet supported). > >>>>> > >>>>> Best, > >>>>> Paul Lam > >>>>> > >>>>>> 2024年5月7日 14:38，Alexey Leonov-Vendrovskiy <vendrov...@gmail.com> > >> 写道： > >>>>>> > >>>>>> Hi everyone, > >>>>>> > >>>>>> PTAL at the proposed FLIP-456: CompiledPlan support for Batch > >>> Execution > >>>>>> Mode. It is pretty self-describing. > >>>>>> > >>>>>> Any thoughts are welcome! > >>>>>> > >>>>>> Thanks, > >>>>>> Alexey > >>>>>> > >>>>>> [1] > >>>>>> > >>>>> > >>> > >> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-456%3A+CompiledPlan+support+for+Batch+Execution+Mode > >>>>>> . > >>>>> > >>>>> > >>> > >> > > > >
