Re: [EXTERNAL] Re: [PATCH 0/16] opentracing: reenable usage of vars to transmit opentracing context
On Fri, Apr 08, 2022 at 04:45:46PM +0200, Miroslav Zagorac wrote: > On 08.04.2022 16:24, Willy Tarreau wrote: > > My concern is essentially this one: is there any risk that a 2.5 user > > currently using opentracing would be hit by a bug introduced with this > > patch ? The code *seems* to be isolated only in the parts that are > > enabled by OT_USE_VARS but I'm asking because you know better than me. > > If context transfer over variables is not used in the opentracing > configuration, the only place where "fixed" functions for variables are used > is when generating ot.uuid variable. This variable is not generated in the > current 2.5 code because it lacks functions for working with variables. > > However, given your explanation of how the thing works with backporting > functionality, we won't break that here. > > If users demand it very much, it could be considered. Now, not to make > trouble for ourselves, we don't need to backport it. OK that's perfectly clear, thank you! Willy
Re: [EXTERNAL] Re: [PATCH 0/16] opentracing: reenable usage of vars to transmit opentracing context
On 08.04.2022 16:24, Willy Tarreau wrote: > My concern is essentially this one: is there any risk that a 2.5 user > currently using opentracing would be hit by a bug introduced with this > patch ? The code *seems* to be isolated only in the parts that are > enabled by OT_USE_VARS but I'm asking because you know better than me. If context transfer over variables is not used in the opentracing configuration, the only place where "fixed" functions for variables are used is when generating ot.uuid variable. This variable is not generated in the current 2.5 code because it lacks functions for working with variables. However, given your explanation of how the thing works with backporting functionality, we won’t break that here. If users demand it very much, it could be considered. Now, not to make trouble for ourselves, we don’t need to backport it. -- Miroslav Zagorac Senior Developer
Re: [EXTERNAL] Re: [PATCH 0/16] opentracing: reenable usage of vars to transmit opentracing context
On Fri, Apr 08, 2022 at 04:04:07PM +0200, Miroslav Zagorac wrote: > > - 0014-MAJOR-opentracing-reenable-usage-of-vars-to-transmit.patch > >=> while I'm fine for 2.6, I'm really not for 2.5 without a big > > compelling reason. It's a feature addition, not a bug fix. > > Either there are currently no opentracing users in 2.5 due to > > this problem and this will serve nobody, or there are and there > > is a significant risk of breaking something for them, which would > > possibly encourage them to revert an update and keep other unrelated > > bugs. Thus is it absolutely mandatory that we backport this to 2.5 ? > > For example, is it currently working so bad that users complain and > > that it prevents them from upgrading from 2.4 to 2.5 ? That's the > > type of situation that can justify a backport of such a patch. > > In my opinion this is not a feature addition but a restore of functionality > that was temporarily disabled due to a change in working with variables > (i.e. saving a hash instead of a name). Sure it was disabled, but *before* the release. This is the essential point here. Users running on 2.5 already do not have it. > This should not bring any drawback to the users as this functionality has > not worked so far in the 2.5 branch and if not used in the configuration it > does not change anything. My concern is essentially this one: is there any risk that a 2.5 user currently using opentracing would be hit by a bug introduced with this patch ? The code *seems* to be isolated only in the parts that are enabled by OT_USE_VARS but I'm asking because you know better than me. Don't get me wrong, you're the OT maintainer, and if as the maintainer you tell me "I need this to be backported", I'll obey. However my job as the project's maintainer is to warn you sufficiently about the fact that this completely violates the standard maintenance rules and that if done it must be done for a valid reason and not just because the patch applies well and probably it can make some users' life better. The general rule of thumb for backports is very simple: "will users yell louder at me for breaking their setup with a patch that wasn't needed than those who're currently missing that patch". If you have a solid reason, you can justify your choice and maintain it (or sometimes revert). If instead you just say "ok ok let's remove it, I just wanted to help", that's rarely perceived as a valid reason for backporting as that was something to be done during the development cycle (as we're doing right now with 2.6). > Maybe some user used this functionality in version 2.4 and it prevented him > from switching to version 2.5 because it doesn't work there? It's possible, but I'm not the best placed to gauge that ;-) > > Regarding the 2.4 series, I'm seeing 5 cleanup patches, but I haven't > > checked if they were needed or not. Normally we do not backport code > > cleanups in stable branches unless they are tiny, riskless and help to > > get other patches backported by providing the required patch context. > > One of the reasons is that some users have patches on top of these > > branches and applying cleanups that are not necessary sometimes causes > > them trouble to re-apply their local patches. It's unlikely that users > > have patched opentracing but that's a general rule, I think you get the > > idea. > > These cleanup patches are not necessary for functionality, so you don't have > to apply them. OK that's perfect. I'm trying a build and merging your set in 2.6 now. Thank you! Willy
Re: [EXTERNAL] Re: [PATCH 0/16] opentracing: reenable usage of vars to transmit opentracing context
On 08.04.2022 15:47, Willy Tarreau wrote: Thanks, but now I'm having further questions: - 0006-BUG-MINOR-opentracing-setting-the-return-value-in-fu.patch - 0016-BUG-BUILD-opentracing-fixed-OT_DEFINE-variable-setti.patch => these ones are bug fixes that are independent on the series, and I verified that they could apply fine before the rest. Do you mind if I do it ? We always want to include fixes before the rest so that bisection is more accurate. I don't have any problems with that. - 0013-EXAMPLES-opentracing-refined-shell-scripts-for-testi.patch => same for this one, I'd put it just after the two fixes above. The same here. - 0008-DOC-opentracing-corrected-comments-in-function-descr.patch => this one, among other things, fixes the description of function flt_ot_smp_init() that was added as part of the series, so at last this part of the fix should be merged with the patch that introduced the function (no need to integrate an incorrect patch and fix it later), and the rest are fixes for the past so I could put it earlier in the stack, with the fixes. Also, I agree. - 0014-MAJOR-opentracing-reenable-usage-of-vars-to-transmit.patch => while I'm fine for 2.6, I'm really not for 2.5 without a big compelling reason. It's a feature addition, not a bug fix. Either there are currently no opentracing users in 2.5 due to this problem and this will serve nobody, or there are and there is a significant risk of breaking something for them, which would possibly encourage them to revert an update and keep other unrelated bugs. Thus is it absolutely mandatory that we backport this to 2.5 ? For example, is it currently working so bad that users complain and that it prevents them from upgrading from 2.4 to 2.5 ? That's the type of situation that can justify a backport of such a patch. In my opinion this is not a feature addition but a restore of functionality that was temporarily disabled due to a change in working with variables (i.e. saving a hash instead of a name). This should not bring any drawback to the users as this functionality has not worked so far in the 2.5 branch and if not used in the configuration it does not change anything. Maybe some user used this functionality in version 2.4 and it prevented him from switching to version 2.5 because it doesn't work there? Please do not send me another series and just let me know your opinion about the points above, I'd like to issue 2.6-dev5 right now, ideally with these patches, and do not want to iterate through yet another series. Ok. Regarding the 2.4 series, I'm seeing 5 cleanup patches, but I haven't checked if they were needed or not. Normally we do not backport code cleanups in stable branches unless they are tiny, riskless and help to get other patches backported by providing the required patch context. One of the reasons is that some users have patches on top of these branches and applying cleanups that are not necessary sometimes causes them trouble to re-apply their local patches. It's unlikely that users have patched opentracing but that's a general rule, I think you get the idea. These cleanup patches are not necessary for functionality, so you don't have to apply them. Thank you for your help. Best regards, -- Miroslav Zagorac Senior Developer
