Hi Xuannan, I have one question more from a strategic point of view: given that we're working on Flink 2.0, wouldn't we actually want to be in a situation where object-reuse is always used and don't make it configurable anymore? IIRC, the only reason why it's a configuration is for backward compatibility.
Best regards, Martijn On Tue, Sep 26, 2023 at 1:32 AM Xuannan Su <suxuanna...@gmail.com> wrote: > > Hi all, > > We would like to revive the discussion and provide a quick update on > the recent work of the FLIP. We have implemented a POC[1], run cases > in the flink-benchmarks[2] against the POC, and verified that many of > the operators in the benchmark will enable object-reuse without code > changes, while the global object-reuse is disabled. > > Please let me know if you have any further comments on the FLIP. If > there are no more comments, we will open the voting in 3 days. > > Best regards, > Xuannan > > [1] https://github.com/apache/flink/pull/22897 > [2] https://github.com/apache/flink-benchmarks > > > On Fri, Jul 7, 2023 at 9:18 AM Dong Lin <lindon...@gmail.com> wrote: > > > > Hi Jing, > > > > Thank you for the suggestion. Yes, we can extend it to support null if in > > the future we find any use-case for this flexibility. > > > > Best, > > Dong > > > > On Thu, Jul 6, 2023 at 7:55 PM Jing Ge <j...@ververica.com> wrote: > > > > > Hi Dong, > > > > > > one scenario I could imagine is that users could enable global object > > > reuse features but force deep copy for some user defined specific > > > functions > > > because of any limitations. But that is only my gut feeling. And agree, we > > > could keep the solution simple for now as FLIP described and upgrade to > > > 3VL > > > once there are such real requirements that are rising. > > > > > > Best regards, > > > Jing > > > > > > On Thu, Jul 6, 2023 at 12:30 PM Dong Lin <lindon...@gmail.com> wrote: > > > > > >> Hi Jing, > > >> > > >> Thank you for the detailed explanation. Please see my reply inline. > > >> > > >> On Thu, Jul 6, 2023 at 3:17 AM Jing Ge <j...@ververica.com> wrote: > > >> > > >>> Hi Xuannan, Hi Dong, > > >>> > > >>> Thanks for your clarification. > > >>> > > >>> @Xuannan > > >>> > > >>> A Jira ticket has been created for the doc update: > > >>> https://issues.apache.org/jira/browse/FLINK-32546 > > >>> > > >>> @Dong > > >>> > > >>> I don't have a concrete example. I just thought about it from a > > >>> conceptual or pattern's perspective. Since we have 1. coarse-grained > > >>> global > > >>> switch(CGS as abbreviation), i.e. the pipeline.object-reuse and 2. > > >>> fine-grained local switch(FGS as abbreviation), i.e. the > > >>> objectReuseCompliant variable for specific operators/functions, there > > >>> will > > >>> be the following patterns with appropriate combinations: > > >>> > > >>> pattern 1: coarse-grained switch only. Local object reuse will be > > >>> controlled by the coarse-grained switch: > > >>> 1.1 cgs == true -> local object reused enabled > > >>> 1.2 cgs == true -> local object reused enabled > > >>> 1.3 cgs == false -> local object reused disabled, i.e. deep copy enabled > > >>> 1.4 cgs == false -> local object reused disabled, i.e. deep copy enabled > > >>> > > >>> afaiu, this is the starting point. I wrote 4 on purpose to make the > > >>> regression check easier. We can consider it as the combinations with > > >>> cgs(true/false) and fgs(true/false) while fgs is ignored. > > >>> > > >>> Now we introduce fine-grained switch. There will be two patterns: > > >>> > > >>> pattern 2: fine-grained switch over coarse-grained switch. > > >>> Coarse-grained switch will be ignored when the local fine-grained switch > > >>> has different value: > > >>> 2.1 cgs == true and fgs == true -> local object reused enabled > > >>> 2.2 cgs == true and fgs == false -> local object reused disabled, i.e. > > >>> deep copy enabled > > >>> 2.3 cgs == false and fgs == true -> local object reused enabled > > >>> 2.4 cgs == false and fgs == false -> local object reused disabled, i.e. > > >>> deep copy enabled > > >>> > > >>> cgs is actually ignored. > > >>> > > >>> Current FLIP is using a slightly different pattern: > > >>> > > >>> pattern 3: fine-grained switch over coarse-grained switch only when > > >>> coarse-grained switch is off, i..e cgs OR fgs: > > >>> 3.1 cgs == true and fgs == true -> local object reused enabled > > >>> 3.2 cgs == true and fgs == false -> local object reused enabled > > >>> 3.3 cgs == false and fgs == true -> local object reused enabled > > >>> 3.4 cgs == false and fgs == false -> local object reused disabled, i.e. > > >>> deep copy enabled > > >>> > > >>> All of those patterns are rational and each has different focus. It > > >>> depends on the real requirement to choose one of them. > > >>> > > >>> As we can see, if fgs is using 2VL, there is a regression between > > >>> pattern 1 and pattern 2. You are absolutely right in this case. That's > > >>> why > > >>> I suggested 3VL, i.e. fgs will have triple values: true, false, > > >>> unknown(e.g. null) > > >>> > > >>> pattern 4: 3VL fgs with the null as init value (again, there are just > > >>> two combination, I made it 4 on purpose): > > >>> 4.1 cgs == true and fgs == null -> local object reused enabled > > >>> 4.2 cgs == true and fgs == null -> local object reused enabled > > >>> 4.3 cgs == false and fgs == null -> local object reused disabled, i.e. > > >>> deep copy enabled > > >>> 4.4 cgs == false and fgs == null -> local object reused disabled, i.e. > > >>> deep copy enabled > > >>> > > >>> Since the default value of fgs is null, pattern 4 is backward compatible > > >>> with pattern 1, which means no regression. > > >>> > > >>> Now we will set value to fgs and follow the pattern 2: > > >>> 4.5 cgs == true and fgs == true -> local object reused enabled > > >>> 4.6 cgs == true and fgs == false -> local object reused disabled, i.e. > > >>> deep copy enabled > > >>> 4.7 cgs == false and fgs == true -> local object reused enabled > > >>> 4.8 cgs == false and fgs == false -> local object reused disabled, i.e. > > >>> deep copy enabled > > >>> > > >>> Pattern 4 contains pattern 3 with the following combinations(force > > >>> enabling local object reuse): > > >>> 4.5 cgs == true and fgs == true -> local object reused enabled > > >>> 4.2 cgs == true and fgs == null -> local object reused enabled > > >>> 4.7 cgs == false and fgs == true -> local object reused enabled > > >>> 4.4 cgs == false and fgs == null -> local object reused disabled, i.e. > > >>> deep copy enabled > > >>> > > >>> Comparing pattern 4 to pattern 3, user will have one additional > > >>> flexibility to control(force disabling) the local object reuse > > >>> capability > > >>> because of 3VL, i.e. 4.2+4.6 vs. 3.2. > > >>> > > >> > > >> I think you are suggesting to allow the user setting fgs to null so that > > >> "user will have one additional flexibility to control(force disabling) > > >> the > > >> local object reuse capability". > > >> > > >> In general, an API that only allows false/true is a bit more complex to > > >> implement than an API that allows false/true/null. All things being > > >> equal, > > >> I believe it is preferred to have a simpler public API. > > >> > > >> I understand you are coming from a conceptual perspective and trying to > > >> make it similar to hierarchical RBAC. However, after thinking through > > >> this, > > >> I still could not find a use-case where we actually need this > > >> flexibility. > > >> In particular, for cases where a user has explicitly configured > > >> pipeline.object-reuse to true, that means the user already knows (or > > >> takes > > >> the responsibility of ensuring) that correctness can be achieved, why > > >> would > > >> the user want to explicitly disable the object-reuse for an operator? > > >> > > >> Regards, > > >> Dong > > >> > > >> > > >>> > > >>> It is commonly used in the hierarchical RBAC to enable more fine-grained > > >>> access control of sub role. > > >>> > > >>> I hope I have been able to explain myself clearly. Looking forward to > > >>> your feedback. > > >>> > > >>> Best regards, > > >>> Jing > > >>> > > >>> > > >>> > > >>> On Wed, Jul 5, 2023 at 12:47 PM Dong Lin <lindon...@gmail.com> wrote: > > >>> > > >>>> Hi Jing, > > >>>> > > >>>> Thanks for the comments! Please find below my comments, which are based > > >>>> on the offline discussion with Xuannan. > > >>>> > > >>>> On Wed, Jul 5, 2023 at 1:36 AM Jing Ge <j...@ververica.com> wrote: > > >>>> > > >>>>> Hi Xuannan, Hi Dong > > >>>>> > > >>>>> Thanks for the Proposal! After reading the FLIP, I'd like to ask some > > >>>>> questions: > > >>>>> > > >>>>> 1. Naming convention for boolean variables. It is recommended to > > >>>>> follow JavaBean [1], i.e. objectReuseCompliant as the variable name > > >>>>> with > > >>>>> isObjectReuseCompliant() and setObjectReuseCompliant() as the > > >>>>> methods' name. > > >>>>> > > >>>>> > > >>>> Good point. We have updated the FLIP as suggested. > > >>>> > > >>>> > > >>>>> > > >>>>> 2. > > >>>>> > > >>>>> - > > >>>>> > > >>>>> *If pipeline.object-reuse is set to true, records emitted by this > > >>>>> operator will be re-used.* > > >>>>> - > > >>>>> > > >>>>> *Otherwise, if getIsObjectReuseCompliant() returns true, records > > >>>>> emitted by this operator will be re-used.* > > >>>>> - > > >>>>> > > >>>>> *Otherwise, records emitted by this operator will be deep-copied > > >>>>> before being given to the next operator in the chain.* > > >>>>> > > >>>>> > > >>>>> If I understand you correctly, the hard coding objectReusedCompliant > > >>>>> should have higher priority over the configuration, the checking logic > > >>>>> should be: > > >>>>> > > >>>>> - > > >>>>> > > >>>>> *If getIsObjectReuseCompliant() returns true, records emitted by > > >>>>> this operator will be re-used.* > > >>>>> - > > >>>>> > > >>>>> *Otherwise, if pipeline.object-reuse is set to true, records > > >>>>> emitted by this operator will be re-used.* > > >>>>> - > > >>>>> > > >>>>> *Otherwise, records emitted by this operator will be deep-copied > > >>>>> before being given to the next operator in the chain.* > > >>>>> > > >>>>> > > >>>>> The results are the same but the checking logics are different, but > > >>>>> there are some additional thoughts, which lead us to the next > > >>>>> question. > > >>>>> > > >>>> > > >>>>> > > >>>> > > >>>>> 3. Current design lets specific operators enable object reuse and > > >>>>> ignore the global config. There could be another thought, on the > > >>>>> contrary: > > >>>>> if an operator has hard coded the objectReuseCompliant as false, i.e. > > >>>>> disable object reuse on purpose, records should not be reused even if > > >>>>> the > > >>>>> global config pipeline.object-reused is set to true, which turns out > > >>>>> that > > >>>>> the objectReuseCompliant could be a triple value logic: ture: force > > >>>>> object > > >>>>> reusing; false: force deep-copying; unknown: depends on > > >>>>> pipeline.object-reuse config. > > >>>>> > > >>>> > > >>>> With the current proposal, if pipeline.object-reused == true and > > >>>> operatorA's objectReuseCompliant == false, Flink will not deep-copy > > >>>> operatorA's output. I think you are suggesting changing the behavior > > >>>> such > > >>>> that Flink should deep-copy the operatorA's output. > > >>>> > > >>>> Could you explain what is the advantage of this approach compared to > > >>>> the approach described in the FLIP? > > >>>> > > >>>> My concern with this approach is that it can cause performance > > >>>> regression. This is an operator's objectReuseCompliant will be false by > > >>>> default unless it is explicitly overridden. For those jobs which are > > >>>> currently configured with pipeline.object-reused = true, these jobs > > >>>> will > > >>>> likely start to have lower performance (due to object deep-copy) after > > >>>> upgrading to the newer Flink version. > > >>>> > > >>>> Best, > > >>>> Dong > > >>>> > > >>>> > > >>>>> > > >>>>> Best regards, > > >>>>> Jing > > >>>>> > > >>>>> > > >>>>> [1] https://en.wikipedia.org/wiki/JavaBeans > > >>>>> > > >>>>> On Mon, Jul 3, 2023 at 4:25 AM Xuannan Su <suxuanna...@gmail.com> > > >>>>> wrote: > > >>>>> > > >>>>>> Hi all, > > >>>>>> > > >>>>>> Dong(cc'ed) and I are opening this thread to discuss our proposal to > > >>>>>> add operator attribute to allow operator to specify support for > > >>>>>> object-reuse [1]. > > >>>>>> > > >>>>>> Currently, the default configuration for pipeline.object-reuse is set > > >>>>>> to false to avoid data corruption, which can result in suboptimal > > >>>>>> performance. We propose adding APIs that operators can utilize to > > >>>>>> inform the Flink runtime whether it is safe to reuse the emitted > > >>>>>> records. This enhancement would enable Flink to maximize its > > >>>>>> performance using the default configuration. > > >>>>>> > > >>>>>> Please refer to the FLIP document for more details about the proposed > > >>>>>> design and implementation. We welcome any feedback and opinions on > > >>>>>> this proposal. > > >>>>>> > > >>>>>> Best regards, > > >>>>>> > > >>>>>> Dong and Xuannan > > >>>>>> > > >>>>>> [1] > > >>>>>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=255073749 > > >>>>>> > > >>>>>
