Hi Martijn, Sorry for the late reply. I don't think it is feasible to always enable object reuse. If I understand correctly, object reuse is disabled by default to guarantee correctness because we cannot assume that the custom operator/function is safe to enable object reuse.
The method proposed in the FLIP is to let the operator inform the Flink runtime whether it is safe to reuse the emitted records. It provides a fine-grained way of controlling the object reuse behavior at the operator level. In the long term, instead of always enabling object reuse, it is better to remove the object-reuse configuration and let the runtime determine whether to enable object reuse for each operator. I hope that addresses your question. Please let me know if you have further comments. Best regards, Xuannan On Fri, Sep 29, 2023 at 8:47 AM Martijn Visser <martijnvis...@apache.org> wrote: > > 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 > > > >>>>>> > > > >>>>>
