Re: [DISCUSS] FLIP-360: Merging ExecutionGraphInfoStore and JobResultStore into a single component

[Gyula Fóra <gy...@gmail.com>]

I am a bit confused by the split in the CompletedJobStore / JobDetailsStore.
Seems like JobDetailsStore is simply a view on top of CompletedJobStore:
 - Maybe we should not call it a store? Is it storing anything?
 - Why couldn't the cleanup triggering be the responsibility of the
CompletedJobStore, wouldn't it make it simpler to have the storage/cleanup
related logic in a simple place?
 - Ideally the JobDetailsStore / JobDetailsProvider could be a very thin
interface exposed by the CompletedJobStore

Gyula

On Sat, Sep 30, 2023 at 2:18 AM Matthias Pohl
<ma...@aiven.io.invalid> wrote:

> Thanks for sharing your thoughts, Shammon FY. I kind of see your point.
>
> Initially, I didn't put much thought into splitting up the interface into
> two because the dispatcher would have been the only component dealing with
> the two interfaces. Having two interfaces wouldn't have added much value
> (in terms of testability and readability, I thought).
>
> But I iterated over the idea once more and came up with a new proposal that
> involves the two components CompletedJobStore and JobDetailsStore. It's not
> 100% what you suggested (because the retrieval of the ExecutionGraphInfo
> still lives in the CompletedJobStore) but the separation makes sense in my
> opinion:
> - The CompletedJobStore deals with the big data that might require
> accessing the disk.
> - The JobDetailsStore handles the small-footprint data that lives in memory
> all the time. Additionally, it takes care of actually deleting the metadata
> of the completed job in both stores if a TTL is configured.
>
> See FLIP-360 [1] with the newly added class and sequence diagrams and
> additional content. I only updated the Interfaces & Methods section (see
> diff [2]).
>
> I'm looking forward to feedback.
>
> Best,
> Matthias
>
> [1]
>
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-360%3A+merging+the+executiongraphinfostore+and+the+jobresultstore+into+a+single+component+completedjobstore
> [2]
>
> https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=263428420&selectedPageVersions=14&selectedPageVersions=13
>
> On Mon, Sep 18, 2023 at 1:20 AM Shammon FY <zj...@gmail.com> wrote:
>
> > Hi Matthias,
> >
> > Thanks for initiating this discussion, and +1 for overall from my side.
> > It's really strange to have two different places to store completed jobs,
> > this also brings about the complexity of Flink. I agree with using a
> > unified instance to store the completed job information.
> >
> > In terms of ability, `ExecutionGraphInfoStore` and `JobResultStore` are
> > different: one is mainly used for information display, and the other is
> for
> > failover. So after unifying storage, can we use different interfaces to
> > meet the different requirements for jobs? Adding all these methods for
> > different components into one interface such as `CompletedJobStore` may
> be
> > a little strange. What do you think?
> >
> > Best,
> > Shammon FY
> >
> >
> >
> > On Fri, Sep 8, 2023 at 8:08 PM Gyula Fóra <gy...@gmail.com> wrote:
> >
> > > Hi Matthias!
> > >
> > > Thank you for the detailed proposal, overall I am in favor of making
> this
> > > unification to simplify the logic and make the integration for external
> > > components more straightforward.
> > > I will try to read through the proposal more carefully next week and
> > > provide some detailed feedback.
> > >
> > > +1
> > >
> > > Thanks
> > > Gyula
> > >
> > > On Fri, Sep 8, 2023 at 8:36 AM Matthias Pohl <matthias.pohl@aiven.io
> > > .invalid>
> > > wrote:
> > >
> > > > Just a bit more elaboration on the question that we need to answer
> > here:
> > > Do
> > > > we want to expose the internal ArchivedExecutionGraph data structure
> > > > through JSON?
> > > >
> > > > - The JSON approach allows the user to have (almost) full access to
> the
> > > > information (that would be otherwise derived from the REST API).
> > > Therefore,
> > > > there's no need to spin up a cluster to access this information.
> > > > Any information that shall be exposed through the REST API needs to
> be
> > > > well-defined in this JSON structure, though. Large parts of the
> > > > ArchivedExecutionGraph data structure (essentially anything that
> shall
> > be
> > > > used to populate the REST API) become public domain, though, which
> puts
> > > > more constraints on this data structure and makes it harder to change
> > it
> > > in
> > > > the future.
> > > >
> > > > - The binary data approach allows us to keep the data structure
> itself
> > > > internal. We have more control over what we want to expose by
> providing
> > > > access points in the ClusterClient (e.g. just add a command to
> extract
> > > the
> > > > external storage path from the file).
> > > >
> > > > - The compromise (i.e. keeping ExecutionGraphInfoStore and
> > JobResultStore
> > > > separate and just expose the checkpoint information next to the
> > JobResult
> > > > in the JobResultStore file) would keep us the closest to the current
> > > state,
> > > > requires the least code changes and the least exposure of internal
> data
> > > > structures. It would allow any system (like the Kubernetes Operator)
> to
> > > > extract the checkpoint's external storage path. But we would still be
> > > stuck
> > > > with kind-of redundant components.
> > > >
> > > > From a user's perspective, I feel like the JSON approach is the best
> > one
> > > > because it gives him/her the most freedom to be independent of Flink
> > > > binaries when handling completed jobs. But I see benefits from a
> Flink
> > > > developer's perspective to not expose the entire data structure but
> use
> > > the
> > > > ClusterClient as an access point.
> > > >
> > > > The last option is my least favorite one: Moving the
> ExecutionGraphInfo
> > > out
> > > > of the JobManager seems to be the right thing to do when thinking
> about
> > > > Flink's vision to become cloud-native.
> > > >
> > > > Just my 2cts on that topic.
> > > > Matthias
> > > >
> > > > On Mon, Sep 4, 2023 at 1:11 PM Matthias Pohl <matthias.pohl@aiven.io
> >
> > > > wrote:
> > > >
> > > > > Hi everyone,
> > > > > I want to open the discussion on FLIP-360 [1]. The goal of this
> FLIP
> > is
> > > > to
> > > > > combine the two very similar components ExecutionGraphInfoStore and
> > > > > JobResultStore into a single component.
> > > > >
> > > > > The benefit of this effort would be to expose the metadata of a
> > > > > globally-terminated job even in cases where the JobManager fails
> > > shortly
> > > > > after the job finished. This is relevant for external checkpoint
> > > > management
> > > > > (like it's done in the Kubernetes Operator) which relies on the
> > > > checkpoint
> > > > > information to be available.
> > > > >
> > > > > More generally, it would allow completed jobs to be listed as part
> of
> > > the
> > > > > Flink cluster even after a JM failover. This would allow users to
> > gain
> > > > more
> > > > > control over finished jobs.
> > > > >
> > > > > The current state of the FLIP doesn't come up with a final
> conclusion
> > > on
> > > > > the serialization format of the data (JSON vs binary). I want to
> > > > emphasize
> > > > > that there's also a third option which keeps both components
> separate
> > > and
> > > > > only exposes the additional checkpoint information through the
> > > > > JobResultStore.
> > > > >
> > > > > I'm looking forward to feedback.
> > > > > Best,
> > > > > Matthias
> > > > >
> > > > > PS: I might be less responsive in the next 2-3 weeks but want to
> > > initiate
> > > > > the discussion, anyway.
> > > > >
> > > > > [1]
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-360%3A+Merging+the+ExecutionGraphInfoStore+and+the+JobResultStore+into+a+single+component+CompletedJobStore
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] FLIP-360: Merging ExecutionGraphInfoStore and JobResultStore into a single component

[Matthias Pohl <ma...@aiven.io.INVALID>]

Hi Gyula,
thanks for joining the discussion. Yeah, I might have gone a bit too far
with it. I guess I was eager to create separate implementations to make use
of the two different interfaces/purposes on a class level to allow more
purpose-centric testing.

I am fine with cutting it back and having separate interfaces being
implemented by the same class as @Shammon proposed. I updated the FLIP
accordingly.

Matthias

On Mon, Oct 23, 2023 at 2:45 PM Gyula Fóra <gy...@gmail.com> wrote:

> I am a bit confused by the split in the CompletedJobStore /
> JobDetailsStore.
> Seems like JobDetailsStore is simply a view on top of CompletedJobStore:
>  - Maybe we should not call it a store? Is it storing anything?
>  - Why couldn't the cleanup triggering be the responsibility of the
> CompletedJobStore, wouldn't it make it simpler to have the storage/cleanup
> related logic in a simple place?
>  - Ideally the JobDetailsStore / JobDetailsProvider could be a very thin
> interface exposed by the CompletedJobStore
>
> Gyula
>
> On Sat, Sep 30, 2023 at 2:18 AM Matthias Pohl
> <ma...@aiven.io.invalid> wrote:
>
> > Thanks for sharing your thoughts, Shammon FY. I kind of see your point.
> >
> > Initially, I didn't put much thought into splitting up the interface into
> > two because the dispatcher would have been the only component dealing
> with
> > the two interfaces. Having two interfaces wouldn't have added much value
> > (in terms of testability and readability, I thought).
> >
> > But I iterated over the idea once more and came up with a new proposal
> that
> > involves the two components CompletedJobStore and JobDetailsStore. It's
> not
> > 100% what you suggested (because the retrieval of the ExecutionGraphInfo
> > still lives in the CompletedJobStore) but the separation makes sense in
> my
> > opinion:
> > - The CompletedJobStore deals with the big data that might require
> > accessing the disk.
> > - The JobDetailsStore handles the small-footprint data that lives in
> memory
> > all the time. Additionally, it takes care of actually deleting the
> metadata
> > of the completed job in both stores if a TTL is configured.
> >
> > See FLIP-360 [1] with the newly added class and sequence diagrams and
> > additional content. I only updated the Interfaces & Methods section (see
> > diff [2]).
> >
> > I'm looking forward to feedback.
> >
> > Best,
> > Matthias
> >
> > [1]
> >
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-360%3A+merging+the+executiongraphinfostore+and+the+jobresultstore+into+a+single+component+completedjobstore
> > [2]
> >
> >
> https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=263428420&selectedPageVersions=14&selectedPageVersions=13
> >
> > On Mon, Sep 18, 2023 at 1:20 AM Shammon FY <zj...@gmail.com> wrote:
> >
> > > Hi Matthias,
> > >
> > > Thanks for initiating this discussion, and +1 for overall from my side.
> > > It's really strange to have two different places to store completed
> jobs,
> > > this also brings about the complexity of Flink. I agree with using a
> > > unified instance to store the completed job information.
> > >
> > > In terms of ability, `ExecutionGraphInfoStore` and `JobResultStore` are
> > > different: one is mainly used for information display, and the other is
> > for
> > > failover. So after unifying storage, can we use different interfaces to
> > > meet the different requirements for jobs? Adding all these methods for
> > > different components into one interface such as `CompletedJobStore` may
> > be
> > > a little strange. What do you think?
> > >
> > > Best,
> > > Shammon FY
> > >
> > >
> > >
> > > On Fri, Sep 8, 2023 at 8:08 PM Gyula Fóra <gy...@gmail.com>
> wrote:
> > >
> > > > Hi Matthias!
> > > >
> > > > Thank you for the detailed proposal, overall I am in favor of making
> > this
> > > > unification to simplify the logic and make the integration for
> external
> > > > components more straightforward.
> > > > I will try to read through the proposal more carefully next week and
> > > > provide some detailed feedback.
> > > >
> > > > +1
> > > >
> > > > Thanks
> > > > Gyula
> > > >
> > > > On Fri, Sep 8, 2023 at 8:36 AM Matthias Pohl <matthias.pohl@aiven.io
> > > > .invalid>
> > > > wrote:
> > > >
> > > > > Just a bit more elaboration on the question that we need to answer
> > > here:
> > > > Do
> > > > > we want to expose the internal ArchivedExecutionGraph data
> structure
> > > > > through JSON?
> > > > >
> > > > > - The JSON approach allows the user to have (almost) full access to
> > the
> > > > > information (that would be otherwise derived from the REST API).
> > > > Therefore,
> > > > > there's no need to spin up a cluster to access this information.
> > > > > Any information that shall be exposed through the REST API needs to
> > be
> > > > > well-defined in this JSON structure, though. Large parts of the
> > > > > ArchivedExecutionGraph data structure (essentially anything that
> > shall
> > > be
> > > > > used to populate the REST API) become public domain, though, which
> > puts
> > > > > more constraints on this data structure and makes it harder to
> change
> > > it
> > > > in
> > > > > the future.
> > > > >
> > > > > - The binary data approach allows us to keep the data structure
> > itself
> > > > > internal. We have more control over what we want to expose by
> > providing
> > > > > access points in the ClusterClient (e.g. just add a command to
> > extract
> > > > the
> > > > > external storage path from the file).
> > > > >
> > > > > - The compromise (i.e. keeping ExecutionGraphInfoStore and
> > > JobResultStore
> > > > > separate and just expose the checkpoint information next to the
> > > JobResult
> > > > > in the JobResultStore file) would keep us the closest to the
> current
> > > > state,
> > > > > requires the least code changes and the least exposure of internal
> > data
> > > > > structures. It would allow any system (like the Kubernetes
> Operator)
> > to
> > > > > extract the checkpoint's external storage path. But we would still
> be
> > > > stuck
> > > > > with kind-of redundant components.
> > > > >
> > > > > From a user's perspective, I feel like the JSON approach is the
> best
> > > one
> > > > > because it gives him/her the most freedom to be independent of
> Flink
> > > > > binaries when handling completed jobs. But I see benefits from a
> > Flink
> > > > > developer's perspective to not expose the entire data structure but
> > use
> > > > the
> > > > > ClusterClient as an access point.
> > > > >
> > > > > The last option is my least favorite one: Moving the
> > ExecutionGraphInfo
> > > > out
> > > > > of the JobManager seems to be the right thing to do when thinking
> > about
> > > > > Flink's vision to become cloud-native.
> > > > >
> > > > > Just my 2cts on that topic.
> > > > > Matthias
> > > > >
> > > > > On Mon, Sep 4, 2023 at 1:11 PM Matthias Pohl <
> matthias.pohl@aiven.io
> > >
> > > > > wrote:
> > > > >
> > > > > > Hi everyone,
> > > > > > I want to open the discussion on FLIP-360 [1]. The goal of this
> > FLIP
> > > is
> > > > > to
> > > > > > combine the two very similar components ExecutionGraphInfoStore
> and
> > > > > > JobResultStore into a single component.
> > > > > >
> > > > > > The benefit of this effort would be to expose the metadata of a
> > > > > > globally-terminated job even in cases where the JobManager fails
> > > > shortly
> > > > > > after the job finished. This is relevant for external checkpoint
> > > > > management
> > > > > > (like it's done in the Kubernetes Operator) which relies on the
> > > > > checkpoint
> > > > > > information to be available.
> > > > > >
> > > > > > More generally, it would allow completed jobs to be listed as
> part
> > of
> > > > the
> > > > > > Flink cluster even after a JM failover. This would allow users to
> > > gain
> > > > > more
> > > > > > control over finished jobs.
> > > > > >
> > > > > > The current state of the FLIP doesn't come up with a final
> > conclusion
> > > > on
> > > > > > the serialization format of the data (JSON vs binary). I want to
> > > > > emphasize
> > > > > > that there's also a third option which keeps both components
> > separate
> > > > and
> > > > > > only exposes the additional checkpoint information through the
> > > > > > JobResultStore.
> > > > > >
> > > > > > I'm looking forward to feedback.
> > > > > > Best,
> > > > > > Matthias
> > > > > >
> > > > > > PS: I might be less responsive in the next 2-3 weeks but want to
> > > > initiate
> > > > > > the discussion, anyway.
> > > > > >
> > > > > > [1]
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-360%3A+Merging+the+ExecutionGraphInfoStore+and+the+JobResultStore+into+a+single+component+CompletedJobStore
> > > > > >
> > > > >
> > > >
> > >
> >
>