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 > > > > > > > > > > > > > > >
