Hi Zakelly, Just for my own clear understanding the suggestion is the following?
state.type=keyed|non-keyed|operator|metadata Where non-keyed == operator? I'm fine with this. BR, G On Thu, Dec 19, 2024 at 10:31 AM Zakelly Lan <zakelly....@gmail.com> wrote: > Hi Gabor, > > The proposed configs look good to me. I also suggest that 'operator' can > serve as an equivalent to 'non-keyed', as non-keyed state is also called > operator state [1]. > > > [1] > > https://nightlies.apache.org/flink/flink-docs-master/docs/dev/datastream/fault-tolerance/state/#operator-state > > Best, > Zakelly > > On Thu, Dec 19, 2024 at 4:46 PM Gabor Somogyi <gabor.g.somo...@gmail.com> > wrote: > > > Hi Zakelly, > > > > Fair point to have short names. > > How about having the following configs then? > > > > connector=savepoint > > state.type=keyed|non-keyed|metadata > > > > This would introduce the proper keyed/non-keyed option + the config pair > > will > > exactly tell that savepoint metadata is going to be read. > > > > BR, > > G > > > > > > On Thu, Dec 19, 2024 at 4:11 AM Zakelly Lan <zakelly....@gmail.com> > wrote: > > > > > Hi Gabor, > > > > > > Thanks for your answers! > > > > > > I'm good with naming it 'savepoint' with an extra option like 'mode' > > > specifying the reading type, or suffixes in names like > > 'savepoint-metadata' > > > for each type of state. For the second option it means the current > > > connector will be named 'savepoint-keyed'? Well I slightly prefer a > short > > > name. > > > > > > For connector='metadata', it seems unclear to me (what is metadata of > > Flink > > > :D), so I don't recommend this option. > > > > > > > > > Best, > > > Zakelly > > > > > > > > > On Wed, Dec 18, 2024 at 10:35 PM Gabor Somogyi < > > gabor.g.somo...@gmail.com> > > > wrote: > > > > > > > Hi Zakelly, > > > > > > > > Thanks for your comments, please find my answers inline. > > > > > > > > BR, > > > > G > > > > > > > > > > > > On Wed, Dec 18, 2024 at 2:42 PM Zakelly Lan <zakelly....@gmail.com> > > > wrote: > > > > > > > > > Hi Gabor, > > > > > > > > > > Thanks for the FLIP! It's very convenient for users to use SQL to > > read > > > > > checkpoint data, so overall I'd +1 for this FLIP. Here are my > > > > > thoughts/questions: > > > > > > > > > > 1. I see this connector is designed to read the > > checkpoint/savepoint > > > > > data, so I'd suggest naming this connector as 'checkpoint' or > > > > > 'savepoint', > > > > > which is more intuitive. The "connector='state'" makes me think > > this > > > > is > > > > > reading the runtime state. Also the 'state.path' can be changed > to > > > > > 'checkpoint.path'. > > > > > > > > > Rename is a possibility, I'm fine with either way in terms of naming. > > > > The only thing what I would consider is that later on it would be > good > > > > to add metadata reading possibility. Such case we can either provide > > > > "savepoint" for data reading and "savepoint_metadata" or simply > > > "metadata" > > > > for meta reading (See 3rd bullet for another approach). Please share > > your > > > > thoughts on this. > > > > > > > > > > > > > 2. IIUC this connector is a bounded source and batch execution > is > > > > > preferred, right? And it cannot read checkpoints while the > > original > > > > job > > > > > is > > > > > running and doing periodic checkpoints. Thus I'd prefer the name > > > > > 'savepoint', as the lifecycle of savepoints is managed by the > user > > > > > instead > > > > > of the Flink job. > > > > > > > > > We can call it savepoint, just agree on the metadata naming as well > to > > > have > > > > an overall picture. > > > > > > > > > > > > > 3. As you pictured, there might be more connectors for states > > other > > > > than > > > > > keyed state, so I'm thinking about providing them in one > connector > > > > with > > > > > an > > > > > option 'state.type' enumerating 'keyed', 'non-keyed' > > ('operator'), > > > > and > > > > > so > > > > > on. Well for the first version this is not a required option. > > Seems > > > > more > > > > > organized. WDYT? > > > > > > > > > Yeah, later on we must provide such config parameter somehow. > > > > Couple of things to consider: > > > > - Maybe we can add "metadata" as possible value here instead of > > > > "connector=X" > > > > - I'm thinking about whether we can come up with something > > > > where we can mix metadata, keyed and non-keyed data in a single SQL > > > > statement. > > > > Without super deep consideration I would say no, but I'm going to > think > > > it > > > > through. > > > > > > > > > > > > > 4. I'm not sure if the general option 'key.format' is needed, > > would > > > > > using 'fields.k.format' while the 'k' is the name of the SQL > > primary > > > > > column > > > > > be feasible? > > > > > > > > > I think that's a nice catch of a simplification. > > > > > > > > > > > > > 5. Reading window state[1] is unsupported, right? > > > > > > > > > Yeah, not yet covered here and can come in a later FLIP. > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > https://nightlies.apache.org/flink/flink-docs-master/docs/libs/state_processor_api/#window-state > > > > > > > > > > Best, > > > > > Zakelly > > > > > > > > > > On Wed, Dec 18, 2024 at 5:46 PM Gabor Somogyi < > > > gabor.g.somo...@gmail.com > > > > > > > > > > wrote: > > > > > > > > > > > Hi All, > > > > > > > > > > > > I'd like to start a discussion of FLIP-496: SQL connector for > keyed > > > > state > > > > > > data [1]. > > > > > > Feel free to add your thoughts to make this feature better. > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-496%3A+SQL+connector+for+keyed+state+data > > > > > > > > > > > > BR, > > > > > > G > > > > > > > > > > > > > > > > > > > > >
