Thanks for the great suggestion and the great discussion. Generally big +1
to this effort.
Some thoughts from my side:
*## Backwards Compatibility*
I think we should really strive to make this non breaking. Maybe we have
new classes / interfaces for StateBackends and CheckpointStorage and let
the existing State Backend classes implement both (and deprecate them)?
In the past, I have gotten some harsh comments from users about breaking
long-time effectively stable APIs, so let's try hard to avoid this (unless
it makes things impossible).
*## Naming*
HashMapStateBackend sounds good to me
Could we rename the SnapshotStorage to CheckpointStorage? Or converge all
methods around "Snapshot"?
I think we already have some confusion from mixing the terms checkpoint and
snapshot and should converge in either direction.
I am slightly leaning towards converging around checkpoints, because that's
the most commonly known term among users as far as I can tell.
Checkpoints are Snapshots. But one could also just call them Checkpoints
and let Savepoints be special Checkpoints.
*## Integrated State / Storage Backends*
There is an idea floating around now and then about a Cassandra backend (or
other K/V store) where the state index and durable location are tightly
intertwined.
However, I think this would not contradict, because it might just mean that
the checkpoint storage is used less (maybe only for savepoints, or for
WALs).
*## Future Fault Tolerance Ideas*
I think this conflicts with none of the future fault tolerance ideas I am
involved with.
Similar to the above, there is always some checkpoint storage involved, for
example for savepoints or for backup/consolidation, so no problem.
Best,
Stephan
On Wed, Sep 16, 2020 at 5:11 PM Aljoscha Krettek <aljos...@apache.org>
wrote:
> I think the mentioned settings should be in the state backend. They
> configure how a certain backend writes to a snapshot storage, but it’s
> still the backend that has the logic and decides.
>
> I think it's a good point, though, to be conscious about those settings.
> I'm sure we can figure out the details during implementation, though.
>
> Best,
> Aljoscha
>
> On 16.09.20 16:54, Seth Wiesman wrote:
> > Hi Congxian,
> >
> > There is an allusion to those configs in the wiki but let me better spell
> > out my thinking. The flink-conf configurations will not change and I
> > believe the java code switches should remain on the state backend
> objects.
> >
> > We are of course not fully disentangling state backends from snapshots
> and
> > these configurations affect how your state backend runs in production. I
> > believe users would find it strange to have configurations like
> > `state.backend.rocksdb.checkpoint.transfer.thred.num` not be part of the
> > EmbeddedRocksdbStateBackend but somewhere else. This then leads to the
> > question, is it better to split configurations between multiple places or
> > not. Users appreciate consistency, and so having all the configurations
> on
> > the state backend objects makes them more discoverable and your
> application
> > easier to reason about.
> >
> > Additionally, I view these as advanced configurations. My hope is most
> > users can simply use the no-arg constructor for a state backend in
> > production. If a user is changing the number of rocksdb transfer threads
> or
> > disabling async checkpoints, they likely know what they are doing.
> >
> > Please let me know if you have any concerns or would like to cancel the
> > vote.
> >
> > Seth
> >
> > On Wed, Sep 16, 2020 at 12:37 AM Congxian Qiu <qcx978132...@gmail.com>
> > wrote:
> >
> >> Sorry for jump late in.
> >>
> >> I like the separation here, this separation makes more user friendly
> now.
> >>
> >> I just wonder how the configuration such as 'state.backend.incremental',
> >> 'state.backend.async' and
> >> `state.backend.rocksdb.checkpoint.transfer.thred.num` will be configured
> >> after the separation, I think these configurations are more related to
> >> snapshots (maybe a little strange to configure these on statebackend
> side).
> >> did not see this on the FLIP wiki currently.
> >>
> >> Best,
> >> Congxian
> >>
> >>
> >> Seth Wiesman <sjwies...@gmail.com> 于2020年9月15日周二 下午9:51写道：
> >>
> >>> Sounds good to me. I'll update the FLIP.
> >>>
> >>> On Tue, Sep 15, 2020 at 8:35 AM Dawid Wysakowicz <
> dwysakow...@apache.org
> >>>
> >>> wrote:
> >>>
> >>>> There is a good number of precedents that introduced backwards
> >>>> incompatible changes to that interface (which is PublicEvolving btw).
> >> We
> >>>> introduced a couple of additional arguments to the
> >>>> createKeyedStateBackend method and later on removed the methods with
> >>>> default implementation for backwards compatibility. I want to
> introduce
> >>>> a backward incompatible change in FLIP-140 (replace the
> >>>> AbstractKeyedStateBackend with an interface). From my perspective we
> >>>> should just do these changes. The impact should be minimal.
> >>>>
> >>>> Best,
> >>>>
> >>>> Dawid
> >>>>
> >>>>
> >>>> On 15/09/2020 15:20, Seth Wiesman wrote:
> >>>>> Hey Dawid,
> >>>>>
> >>>>> I didn't want to break compatibility but if there is precedent and
> >>>> everyone
> >>>>> is ok with it then I'm +1.
> >>>>>
> >>>>> Seth
> >>>>>
> >>>>> On Tue, Sep 15, 2020 at 2:22 AM Dawid Wysakowicz <
> >>> dwysakow...@apache.org
> >>>>>
> >>>>> wrote:
> >>>>>
> >>>>>> Sorry for joining so late.
> >>>>>>
> >>>>>> Generally speaking I like this idea very much!
> >>>>>>
> >>>>>> I have one idea about the StateBackend interface. Could we instead
> >> of
> >>>>>> adding a flag method boolean isLegacyStateBackend remove the
> >>>>>> checkpointstorage related methods from StateBackend right away? The
> >>>>>> old/legacy implementations could then implement both StateBackend
> >> and
> >>>>>> SnapshotStorage. In turn in the method env.setStateBackend we could
> >>> do:
> >>>>>>
> >>>>>> setStateBackend(StateBackend backend) {
> >>>>>>
> >>>>>> this.stateBackend = backend;
> >>>>>>
> >>>>>> if (backend instanceof SnapshotStorage) {
> >>>>>>
> >>>>>> this.setSnapshotStorage(backend);
> >>>>>>
> >>>>>> }
> >>>>>>
> >>>>>> }
> >>>>>>
> >>>>>> This has the benefit that we could already get rid off the methods
> >>> from
> >>>>>> StateBackend which would be problematic in the new implementations
> >>> (such
> >>>>>> as e.g. HashMapStateBackend - what would you return there? null?). I
> >>>>>> know this would break the interface, but StateBackend is actually
> >>> quite
> >>>>>> internal, we did it quite freely in the past, and I don't think
> >> there
> >>>>>> are many custom state implementation in the wild. And even if there
> >>> are
> >>>>>> some the workaround is as easy as simply adding implements
> >>>> SnapshotStorage.
> >>>>>>
> >>>>>> Best,
> >>>>>>
> >>>>>> Dawid
> >>>>>>
> >>>>>> On 11/09/2020 16:48, Aljoscha Krettek wrote:
> >>>>>>> I could try and come up with a longer name if you need it ... ;-)
> >>>>>>>
> >>>>>>> Aljoscha
> >>>>>>>
> >>>>>>> On 11.09.20 16:25, Seth Wiesman wrote:
> >>>>>>>> Having thought about it more, HashMapStateBackend has won me over.
> >>>> I'll
> >>>>>>>> update the FLIP. If there aren't any more comments I'll open it up
> >>> for
> >>>>>>>> voting on monday.
> >>>>>>>>
> >>>>>>>> Seth
> >>>>>>>>
> >>>>>>>> On Wed, Sep 9, 2020 at 9:09 AM Seth Wiesman <sjwies...@gmail.com>
> >>>>>> wrote:
> >>>>>>>>> @Yun yes, this is really about making CheckpointStorage an
> >>> orthogonal
> >>>>>>>>> concept. I think we can remain pragmatic and keep state-backend
> >>>>>>>>> specific
> >>>>>>>>> configurations (async, incremental, etc) in the state backend
> >>>>>>>>> themselves. I
> >>>>>>>>> view these as more advanced configurations and by the time
> >> someone
> >>> is
> >>>>>>>>> changing the defaults they likely understand what is going on. My
> >>>>>>>>> goal is
> >>>>>>>>> to help on-board users and so long as each state backend has a
> >>> no-arg
> >>>>>>>>> default constructor that works for many users I think we've
> >>> achieved
> >>>>>>>>> that
> >>>>>>>>> goal.
> >>>>>>>>>
> >>>>>>>>> Regarding the checkpoint coordinator, that makes sense but I will
> >>>>>>>>> consider
> >>>>>>>>> out of the scope of this FLIP. I want to focus on simplifying
> >> APIs.
> >>>>>>>>>
> >>>>>>>>> @Aljoscha Krettek <aljos...@apache.org>
> >>>>>>>>>
> >>>>>>>>> My feeling is that state backends and checkpointing are going to
> >> be
> >>>>>>>>> integral to Flink for many years, regardless or other
> >> enhancements
> >>>>>>>>> so this
> >>>>>>>>> change is still valuable.
> >>>>>>>>>
> >>>>>>>>> Since this is a FLIP about improving the user api I'm happy to
> >>>> bikeshed
> >>>>>>>>> the names a little more than normal. HashMap makes sense, my
> >> other
> >>>>>>>>> thought
> >>>>>>>>> was InMemory.
> >>>>>>>>>
> >>>>>>>>> Seth
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Wed, Sep 9, 2020 at 8:04 AM Aljoscha Krettek <
> >>> aljos...@apache.org
> >>>>>
> >>>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>> I like it a lot!
> >>>>>>>>>>
> >>>>>>>>>> I think it makes sense to clean this up despite the planned new
> >>>>>>>>>> fault-tolerance mechanisms. In the future, users will decide
> >> which
> >>>>>>>>>> mechanism to use and I can imagine that a lot of them will keep
> >>>> using
> >>>>>>>>>> the current mechanism for quite a while to come. But I'm happy
> >> to
> >>>>>>>>>> yield
> >>>>>>>>>> to Stephan's opinion here, he knows more about the progress of
> >>> that
> >>>>>>>>>> work.
> >>>>>>>>>>
> >>>>>>>>>> The one nitpick I have is about naming: will users understand
> >>>>>>>>>> OnHeapStateBackend? I mean, do they know what on-heap/off-heap
> >>>>>>>>>> memory is
> >>>>>>>>>> and the tradeoffs? An alternative could be HashMapStateBackend,
> >>>>>>>>>> because
> >>>>>>>>>> that's essentially what it is. I wouldn't block anything on
> >> this,
> >>>>>>>>>> though.
> >>>>>>>>>>
> >>>>>>>>>> Aljoscha
> >>>>>>>>>>
> >>>>>>>>>> On 09.09.20 10:05, Konstantin Knauf wrote:
> >>>>>>>>>>> Thanks for the initiative. Big +1. Would be interested to hear
> >> if
> >>>> the
> >>>>>>>>>>> proposed interfaces still make sense in the face of the new
> >>>>>>>>>> fault-tolerance
> >>>>>>>>>>> work that is planned. Stephan/Piotr will know.
> >>>>>>>>>>>
> >>>>>>>>>>> On Tue, Sep 8, 2020 at 7:05 PM Seth Wiesman <
> >> sjwies...@gmail.com
> >>>>
> >>>>>>>>>> wrote:
> >>>>>>>>>>>> Hi Devs,
> >>>>>>>>>>>>
> >>>>>>>>>>>> I'd like to propose an update to how state backends and
> >>> checkpoint
> >>>>>>>>>> storage
> >>>>>>>>>>>> are configured to help users better understand Flink.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Apache Flink's durability story is a mystery to many users.
> >> One
> >>>>>>>>>>>> of the
> >>>>>>>>>> most
> >>>>>>>>>>>> common recurring questions from users comes from not
> >>>>>>>>>>>> understanding the
> >>>>>>>>>>>> relationship between state, state backends, and snapshots.
> >> Some
> >>>>>>>>>>>> of this
> >>>>>>>>>>>> confusion can be abated with learning material but the
> >> question
> >>>>>>>>>>>> is so
> >>>>>>>>>>>> pervasive that we believe Flink’s user APIs should be better
> >>>>>>>>>> communicate
> >>>>>>>>>>>> what different components are responsible for.
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>
> >>>>
> >>>
> >>
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-142%3A+Disentangle+StateBackends+from+Checkpointing
> >>>>>>>>>>>>
> >>>>>>>>>>>> I look forward to a healthy discussion.
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Seth
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>
> >>>>
> >>>>
> >>>
> >>
> >
>
>
