Thanks for writing this FLIP Timo. I think this will be a very important improvement for Flink and our SQL user :-)
Similar to Francesco I would like to understand the statement > For simplification of the design, we assume that upgrades use a step size of a single minor version. We don't guarantee skipping minor versions (e.g. 1.11 to 1.14). a bit better. Is it because Flink does not guarantee that a savepoint created by version 1.x can be directly recovered by version 1.y with x + 1 < y but users might have to go through a cascade of upgrades? From how I understand your proposal, the compiled plan won't be changed after being written initially. Hence, I would assume that for the plan alone Flink will have to give backwards compatibility guarantees for all versions. Am I understanding this part correctly? Cheers, Till On Thu, Nov 25, 2021 at 4:55 PM Francesco Guardiani <france...@ververica.com> wrote: > Hi Timo, > > Thanks for putting this amazing work together, I have some > considerations/questions > about the FLIP: > *Proposed changes #6*: Other than defining this rule of thumb, we must > also make sure > that compiling plans with these objects that cannot be serialized in the > plan must fail hard, > so users don't bite themselves with such issues, or at least we need to > output warning > logs. In general, whenever the user is trying to use the CompiledPlan APIs > and at the same > time, they're trying to do something "illegal" for the plan, we should > immediately either > log or fail depending on the issue, in order to avoid any surprises once > the user upgrades. > I would also say the same for things like registering a function, > registering a DataStream, > and for every other thing which won't end up in the plan, we should log > such info to the > user by default. > > *General JSON Plan Assumptions #9:* When thinking to connectors and > formats, I think > it's reasonable to assume and keep out of the feature design that no > feature/ability can > deleted from a connector/format. I also don't think new features/abilities > can influence > this FLIP as well, given the plan is static, so if for example, > MyCoolTableSink in the next > flink version implements SupportsProjectionsPushDown, then it shouldn't be > a problem > for the upgrade story since the plan is still configured as computed from > the previous flink > version. What worries me is breaking changes, in particular behavioural > changes that > might happen in connectors/formats. Although this argument doesn't seem > relevant for > the connectors shipped by the flink project itself, because we try to keep > them as stable as > possible and avoid eventual breaking changes, it's compelling to external > connectors and > formats, which might be decoupled from the flink release cycle and might > have different > backward compatibility guarantees. It's totally reasonable if we don't > want to tackle it in > this first iteration of the feature, but it's something we need to keep in > mind for the future. > > > *Functions:* It's not clear to me what you mean for "identifier", because > then somewhere > else in the same context you talk about "name". Are we talking about the > function name > or the function complete signature? Let's assume for example we have these > function > definitions: > > > * TO_TIMESTAMP_LTZ(BIGINT) > * TO_TIMESTAMP_LTZ(STRING) > * TO_TIMESTAMP_LTZ(STRING, STRING) > > These for me are very different functions with different implementations, > where each of > them might evolve separately at a different pace. Hence when we store them > in the json > plan we should perhaps use a logically defined unique id like > /bigIntToTimestamp/, / > stringToTimestamp/ and /stringToTimestampWithFormat/. This also solves the > issue of > correctly referencing the functions when restoring the plan, without > running again the > inference logic (which might have been changed in the meantime) and it > might also solve > the versioning, that is the function identifier can contain the function > version like / > stringToTimestampWithFormat_1_1 /or /stringToTimestampWithFormat_1_2/. An > alternative could be to use the string signature representation, which > might not be trivial > to compute, given the complexity of our type inference logic. > > *The term "JSON plan"*: I think we should rather keep JSON out of the > concept and just > name it "Compiled Plan" (like the proposed API) or something similar, as I > see how in > future we might decide to support/modify our persistence format to > something more > efficient storage wise like BSON. For example, I would rename / > CompiledPlan.fromJsonFile/ to simply /CompiledPlan.fromFile/. > > *Who is the owner of the plan file?* I asked myself this question when > reading this: > > > > For simplification of the design, we assume that upgrades use a step > size of a single > minor version. We don't guarantee skipping minor versions (e.g. 1.11 to > 1.14). > > My understanding of this statement is that a user can upgrade between > minors but then > following all the minors, the same query can remain up and running. E.g. I > upgrade from > 1.15 to 1.16, and then from 1.16 to 1.17 and I still expect my original > query to work > without recomputing the plan. This necessarily means that at some point in > future > releases we'll need some basic "migration" tool to keep the queries up and > running, > ending up modifying the compiled plan. So I guess flink should write it > back in the original > plan file, perhaps doing a backup of the previous one? Can you please > clarify this aspect? > > Except these considerations, the proposal looks good to me and I'm eagerly > waiting to see > it in play. > > Thanks, > FG > > > -- > Francesco Guardiani | Software Engineer > france...@ververica.com[1] > > Follow us @VervericaData > -- > Join Flink Forward[2] - The Apache Flink Conference > Stream Processing | Event Driven | Real Time > -- > Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany > -- > Ververica GmbH > Registered at Amtsgericht Charlottenburg: HRB 158244 B > Managing Directors: Karl Anton Wehner, Holger Temme, Yip Park Tung Jason, > Jinwei (Kevin) > Zhang > > -------- > [1] mailto:france...@ververica.com > [2] https://flink-forward.org/ >
