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 <gyula.f...@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 > <matthias.p...@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 <zjur...@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 <gyula.f...@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.p...@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.p...@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 > > > > > > > > > > > > > > > > > > > > >
