Remap plugin DSO reloading - enabling and disabling

[Alan Carroll <so...@verizonmedia.com>]

As part of the ATS 9 upgrade, a feature was added so that remap plugins
could have their DSO reloaded. This means not just the configuration, but
the implementation itself. While very useful, this has some
unfortunate side effects with plugins that are used in both a global and
remap context. To alleviate this, a configuration variable as added to
disable the feature.

Although reasonable, this is a rather heavy handed way to deal with the
problem. What would be better is the ability to reload the DSO or not on a
per remap plugin basis. I have a few ways this could be done:

1) Add the keyword "@global" to "remap.config". This would behave exactly
as "@plugin" except it would prohibit reloading of the DSO for that plugin.

2) Have the remap reload configuration check to see if the plugin is also a
global plugin and disable remap DSO reload for that plugin.

3) Add a flag to the global plugin registry information which can be set
during TSPluginInit which disables DSO reloading for that plugin, should it
occur in "remap.config". This is similar to (2) but requires a  plugin to
prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and
would only be valid when called from TSPluginInit.

4) As (3), except the flag is set by default and must be cleared to enable
DSO reloading in "remap.config".

I'm willing to see if I can make this work, but I would like to have some
feedback on the preferred approach first.

Re: Remap plugin DSO reloading - enabling and disabling

[Sudheer Vinukonda <su...@yahoo.com>]

 Hey Alan,
I do think "let user decide" is actually very relevant and important in this context. 
Some users (e.g us), like to keep the entire set (Core + Plugins + Config) as one unit and deploy/manage them together. 
While it may not be flexible enough to be able to update a plugin at a time, there are several benefits to this approach (for e.g integration tests, we have plugins that are owned by multiple partner teams and we simply don't want to update those plugins individually without ensuring each version of it gels well with the rest of the bundle). 
But, we could on the other hand want some plugins that we own directly, reloadable. And the fact that static variables didn't work anymore took us by surprise mainly because we weren't aware of it (totally our fault). Now that we understand that it is somewhat of an expected/intended behavior, we'd make necessary adjustments (e,g updating the plugin) if we want to use the reload feature.
So, it's not necessarily about whether the user is qualified to make a decision, rather a matter of organizational structure and policy for how ATS is operated. We could clearly document the behavior and provide recommendations to the users.
Does it make sense?
Thanks!

Sudheer
    On Thursday, May 7, 2020, 07:12:40 PM PDT, Alan Carroll <so...@verizonmedia.com> wrote:  
 
 Leif;
If the plugin can be global or remap but not both, I don't see why (2) limits anything. The entire issue is irrelevant for such plugins, because the situation of reloading the remap DSO but not the global cannot occur, In fact, option (3) or (4) would enable detecting this and issuing a warning.
Approach (1) was my first thought, but I think the problem there is whether the plugin can work as a global and a reloadable remap is a property of the plugin implementation, not any particular remap rule. That is, for a specific plugin, there's really no choice about whether to use "@plugin" or "@global" - the configuration must get it right or the plugin crashes. Every time. Every rule. It is for this reason I disagree with Leif's view the user should decide. The user's opinion is irrelevant - the plugin works in this mode, or it doesn't. And as our friends at LinkedIn discovered, some rather basic C++ decisions (such as using static variables) will prevent a plugin from working in this mode. On the other hand, if the plugin uses the "User Args" feature then it can work, in which case what's the point of disabling the DSO reload? Unless the plugin implementor is concerned about code skew between the global and remap versions, which again the user is not qualified to decide.
My personal preference is (3), but I suspect after mysterious crashes with plugins, we will have been happier with (4).
On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda <su...@yahoo.com> wrote:

 +1 on the general idea to make the reloadability customizable per plugin.
However, I think it'd be more simple, cleaner and intuitive to not tie it to whether or not a plugin is used both as a global and remap plugin.
In other words, approach (1) below but, instead of calling it "@global", we could add a param which says "@reloadable=false" (the default value for "@reloadable" can be "true").
The same param can then be used, when we eventually add relodability to global plugins as well.
Thoughts?



    On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <so...@verizonmedia.com> wrote:  
 
 As part of the ATS 9 upgrade, a feature was added so that remap plugins could have their DSO reloaded. This means not just the configuration, but the implementation itself. While very useful, this has some unfortunate side effects with plugins that are used in both a global and remap context. To alleviate this, a configuration variable as added to disable the feature.
Although reasonable, this is a rather heavy handed way to deal with the problem. What would be better is the ability to reload the DSO or not on a per remap plugin basis. I have a few ways this could be done:
1) Add the keyword "@global" to "remap.config". This would behave exactly as "@plugin" except it would prohibit reloading of the DSO for that plugin.
2) Have the remap reload configuration check to see if the plugin is also a global plugin and disable remap DSO reload for that plugin.
3) Add a flag to the global plugin registry information which can be set during TSPluginInit which disables DSO reloading for that plugin, should it occur in "remap.config". This is similar to (2) but requires a  plugin to prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and would only be valid when called from TSPluginInit.
4) As (3), except the flag is set by default and must be cleared to enable DSO reloading in "remap.config".
I'm willing to see if I can make this work, but I would like to have some feedback on the preferred approach first.
  
  

Re: Remap plugin DSO reloading - enabling and disabling

[Sudheer Vinukonda <su...@yahoo.com>]

Sure, Yeah, I get that issue. 

Some options that may be considered are

1) Enforce consistent usage of “@reloadable” for a given plugin, fail to load when an inconsistency is detected.
2) Slightly more forgiving approach - Only use the first mention of “@reloadable” for a given plugin and ignore the rest (with a WARN or ERROR log for the inconsistency)
3) Just move the @reloadable state for a plugin to an entirely separate config file, say, “plugin_properties.config”

Thoughts?





> On May 8, 2020, at 11:29 AM, Alan Carroll <so...@verizonmedia.com> wrote:
> 
> 
> Consider a situation with option (1) with two remap rules:
> 
> map http://example.one http://example.one @plugin=txn_box.so @reloadable=false blah blah blah
> map http://example.two http://example.two @plugin=txn_box.so @reloadable=true blah blah
> 
> Does that DSO get reloaded on a reload of "remap.config"?
> 
> 
>> On Fri, May 8, 2020 at 9:58 AM Sudheer Vinukonda <su...@yahoo.com> wrote:
>> Ah, true. I get the misunderstanding now. Yeah, I don’t mean to have reloadable flexibility per remap line either, but just per “(remap)plugin”.
>> 
>> And the only point I was trying to make was to let that the flexibility be determined by the user and not implicitly by the fact that a plugin was used in mixed mode. And yeah sorry, I totally missed the problem with making it a remap level param instead of a plugin level param. So, I still prefer your approach 1, except it’d be clearer if it’s named something more obvious indicating non-reload ability than “@global” (but, naming is hard and I can’t think of a short/succinct better name :()
>> 
>> 
>> 
>>>> On May 8, 2020, at 7:33 AM, Alan Carroll <so...@verizonmedia.com> wrote:
>>>> 
>>> 
>>> Sudheer, I understand the point you are making, I just consider it irrelevant. Let me give Leif an example to illustrate why - TxnBox. It shares data between the global and remap configurations at run time via static variables. If you enable remap DSO reloading for TxnBox, it will crash on the first transaction that hits a remap rule. It doesn't matter if it's actually been reloaded or not. However your organization does plugin updates, TxnBox will still crash in that situation. Even in your example, Sudheer, there's no _choice_ about whether a particular plugin can be DSO reloaded, it's a result of the implementation. As you yourself write, you can't enable it for those plugins without changing the code. No configuration cleverness will get around that.
>>> 
>>> For plugins that do support DSO reloading, the enablement is still per plugin, not per remap rule. Moreover, if we went with option (3) it would be simple to have to plugin support a configuration / load time option to enable or disable DSO reloading. In general, if the plugin can be DSO reloaded, it's unclear why it shouldn't be except in unusual circumstances which are depending on the plugin implementation.
>>> 
>>> For Sudheer, I remain unclear on what exact flexibility you want, given the constraints created by a specific plugin's implementation. I've re-read your note and AFAICT it assumes doing DSO reload or not *per plugin*, which is also my point. I dislike (1) because it makes no sense to me to have this change between remap rules for a specific plugin. I think it's better to have the plugin decide if that's possible and, if needed, provide configuration to disable it if needed. Speaking specifically for TxnBox, I must forbid you from enabling DSO reloading. Even in your case, it might be reasonable to have this for plugins that you have not yet updated (which is actually the case with TxnBox - I'm limited by a requirement for ATS 7 compatibility, so I can't change that feature at the current time).
>>> 
>>>> On Thu, May 7, 2020 at 10:11 PM Leif Hedstrom <zw...@apache.org> wrote:
>>>> 
>>>> 
>>>>> On May 7, 2020, at 8:12 PM, Alan Carroll <so...@verizonmedia.com> wrote:
>>>>> 
>>>>> Leif;
>>>>> 
>>>>> If the plugin can be global or remap but not both, I don't see why (2) limits anything. The entire issue is irrelevant for such plugins, because the situation of reloading the remap DSO but not the global cannot occur, In fact, option (3) or (4) would enable detecting this and issuing a warning.
>>>> 
>>>> Ah yes, good point. However, still the same problem, one can very much want to use say header_rewrite as both global and remap plugin at the same time, and be fine with the fact that it doesn’t reload as a “global”, but you want it to reload as a remap. We use that plugin in this way for example. 
>>>> 
>>>> I still feel that option 2) is a bad option, but I’m ok with the others (still with a preference towards #1). I think a finer granular control mechanism here is a good idea.
>>>> 
>>>> I’d also be curious to hear which of the core plugins are having problems here, in most cases, there’s a no dependency between the global instantiation, and the per remap instantiation. Sudheer and LinkedIn have many internal plugins that do experience this problem however, so I’m guessing that maybe you have similar custom internal plugins?
>>>> 
>>>> Cheers,
>>>> 
>>>> — Leif
>>>> 
>>>>> 
>>>>> Approach (1) was my first thought, but I think the problem there is whether the plugin can work as a global and a reloadable remap is a property of the plugin implementation, not any particular remap rule. That is, for a specific plugin, there's really no choice about whether to use "@plugin" or "@global" - the configuration must get it right or the plugin crashes. Every time. Every rule. It is for this reason I disagree with Leif's view the user should decide. The user's opinion is irrelevant - the plugin works in this mode, or it doesn't. And as our friends at LinkedIn discovered, some rather basic C++ decisions (such as using static variables) will prevent a plugin from working in this mode. On the other hand, if the plugin uses the "User Args" feature then it can work, in which case what's the point of disabling the DSO reload? Unless the plugin implementor is concerned about code skew between the global and remap versions, which again the user is not qualified to decide.
>>>>> 
>>>>> My personal preference is (3), but I suspect after mysterious crashes with plugins, we will have been happier with (4).
>>>>> 
>>>>>> On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda <su...@yahoo.com> wrote:
>>>>>> +1 on the general idea to make the reloadability customizable per plugin.
>>>>>> 
>>>>>> However, I think it'd be more simple, cleaner and intuitive to not tie it to whether or not a plugin is used both as a global and remap plugin.
>>>>>> 
>>>>>> In other words, approach (1) below but, instead of calling it "@global", we could add a param which says "@reloadable=false" (the default value for "@reloadable" can be "true").
>>>>>> 
>>>>>> The same param can then be used, when we eventually add relodability to global plugins as well.
>>>>>> 
>>>>>> Thoughts?
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <so...@verizonmedia.com> wrote:
>>>>>> 
>>>>>> 
>>>>>> As part of the ATS 9 upgrade, a feature was added so that remap plugins could have their DSO reloaded. This means not just the configuration, but the implementation itself. While very useful, this has some unfortunate side effects with plugins that are used in both a global and remap context. To alleviate this, a configuration variable as added to disable the feature.
>>>>>> 
>>>>>> Although reasonable, this is a rather heavy handed way to deal with the problem. What would be better is the ability to reload the DSO or not on a per remap plugin basis. I have a few ways this could be done:
>>>>>> 
>>>>>> 1) Add the keyword "@global" to "remap.config". This would behave exactly as "@plugin" except it would prohibit reloading of the DSO for that plugin.
>>>>>> 
>>>>>> 2) Have the remap reload configuration check to see if the plugin is also a global plugin and disable remap DSO reload for that plugin.
>>>>>> 
>>>>>> 3) Add a flag to the global plugin registry information which can be set during TSPluginInit which disables DSO reloading for that plugin, should it occur in "remap.config". This is similar to (2) but requires a  plugin to prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and would only be valid when called from TSPluginInit.
>>>>>> 
>>>>>> 4) As (3), except the flag is set by default and must be cleared to enable DSO reloading in "remap.config".
>>>>>> 
>>>>>> I'm willing to see if I can make this work, but I would like to have some feedback on the preferred approach first.
>>>>>> 
>>>> 

Re: Remap plugin DSO reloading - enabling and disabling

[Leif Hedstrom <zw...@apache.org>]

> On May 8, 2020, at 2:17 PM, Leif Hedstrom <zw...@apache.org> wrote:
> 
> 
> 
>> On May 8, 2020, at 12:29 PM, Alan Carroll <solidwallofcode@verizonmedia.com <ma...@verizonmedia.com>> wrote:
>> 
>> Consider a situation with option (1) with two remap rules:
>> 
>> map http://example.one <http://example.one/> http://example.one <http://example.one/> @plugin=txn_box.so @reloadable=false blah blah blah
>> map http://example.two <http://example.two/> http://example.two <http://example.two/> @plugin=txn_box.so @reloadable=true blah blah
>> 
>> Does that DSO get reloaded on a reload of "remap.config"?
> 
> 
> It should get reloaded for the second, not for the first. As far as I understand, this is fine, the Gancho made the code such that as long as something uses some version of a plugin, it will be kept forever.

Now, if this gets too complicated (but I don’t see why, the old remap is still available when the new one is being created), I’d say produce an Error().

— Leif

> 
> — Leif
> 
> 
> 
>> 
>> 
>> On Fri, May 8, 2020 at 9:58 AM Sudheer Vinukonda <sudheervinukonda@yahoo.com <ma...@yahoo.com>> wrote:
>> Ah, true. I get the misunderstanding now. Yeah, I don’t mean to have reloadable flexibility per remap line either, but just per “(remap)plugin”.
>> 
>> And the only point I was trying to make was to let that the flexibility be determined by the user and not implicitly by the fact that a plugin was used in mixed mode. And yeah sorry, I totally missed the problem with making it a remap level param instead of a plugin level param. So, I still prefer your approach 1, except it’d be clearer if it’s named something more obvious indicating non-reload ability than “@global” (but, naming is hard and I can’t think of a short/succinct better name :()
>> 
>> 
>> 
>>> On May 8, 2020, at 7:33 AM, Alan Carroll <solidwallofcode@verizonmedia.com <ma...@verizonmedia.com>> wrote:
>>> 
>>> 
>>> Sudheer, I understand the point you are making, I just consider it irrelevant. Let me give Leif an example to illustrate why - TxnBox. It shares data between the global and remap configurations at run time via static variables. If you enable remap DSO reloading for TxnBox, it will crash on the first transaction that hits a remap rule. It doesn't matter if it's actually been reloaded or not. However your organization does plugin updates, TxnBox will still crash in that situation. Even in your example, Sudheer, there's no _choice_ about whether a particular plugin can be DSO reloaded, it's a result of the implementation. As you yourself write, you can't enable it for those plugins without changing the code. No configuration cleverness will get around that.
>>> 
>>> For plugins that do support DSO reloading, the enablement is still per plugin, not per remap rule. Moreover, if we went with option (3) it would be simple to have to plugin support a configuration / load time option to enable or disable DSO reloading. In general, if the plugin can be DSO reloaded, it's unclear why it shouldn't be except in unusual circumstances which are depending on the plugin implementation.
>>> 
>>> For Sudheer, I remain unclear on what exact flexibility you want, given the constraints created by a specific plugin's implementation. I've re-read your note and AFAICT it assumes doing DSO reload or not *per plugin*, which is also my point. I dislike (1) because it makes no sense to me to have this change between remap rules for a specific plugin. I think it's better to have the plugin decide if that's possible and, if needed, provide configuration to disable it if needed. Speaking specifically for TxnBox, I must forbid you from enabling DSO reloading. Even in your case, it might be reasonable to have this for plugins that you have not yet updated (which is actually the case with TxnBox - I'm limited by a requirement for ATS 7 compatibility, so I can't change that feature at the current time).
>>> 
>>> On Thu, May 7, 2020 at 10:11 PM Leif Hedstrom <zwoop@apache.org <ma...@apache.org>> wrote:
>>> 
>>> 
>>>> On May 7, 2020, at 8:12 PM, Alan Carroll <solidwallofcode@verizonmedia.com <ma...@verizonmedia.com>> wrote:
>>>> 
>>>> Leif;
>>>> 
>>>> If the plugin can be global or remap but not both, I don't see why (2) limits anything. The entire issue is irrelevant for such plugins, because the situation of reloading the remap DSO but not the global cannot occur, In fact, option (3) or (4) would enable detecting this and issuing a warning.
>>> 
>>> Ah yes, good point. However, still the same problem, one can very much want to use say header_rewrite as both global and remap plugin at the same time, and be fine with the fact that it doesn’t reload as a “global”, but you want it to reload as a remap. We use that plugin in this way for example. 
>>> 
>>> I still feel that option 2) is a bad option, but I’m ok with the others (still with a preference towards #1). I think a finer granular control mechanism here is a good idea.
>>> 
>>> I’d also be curious to hear which of the core plugins are having problems here, in most cases, there’s a no dependency between the global instantiation, and the per remap instantiation. Sudheer and LinkedIn have many internal plugins that do experience this problem however, so I’m guessing that maybe you have similar custom internal plugins?
>>> 
>>> Cheers,
>>> 
>>> — Leif
>>> 
>>>> 
>>>> Approach (1) was my first thought, but I think the problem there is whether the plugin can work as a global and a reloadable remap is a property of the plugin implementation, not any particular remap rule. That is, for a specific plugin, there's really no choice about whether to use "@plugin" or "@global" - the configuration must get it right or the plugin crashes. Every time. Every rule. It is for this reason I disagree with Leif's view the user should decide. The user's opinion is irrelevant - the plugin works in this mode, or it doesn't. And as our friends at LinkedIn discovered, some rather basic C++ decisions (such as using static variables) will prevent a plugin from working in this mode. On the other hand, if the plugin uses the "User Args" feature then it can work, in which case what's the point of disabling the DSO reload? Unless the plugin implementor is concerned about code skew between the global and remap versions, which again the user is not qualified to decide.
>>>> 
>>>> My personal preference is (3), but I suspect after mysterious crashes with plugins, we will have been happier with (4).
>>>> 
>>>> On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda <sudheervinukonda@yahoo.com <ma...@yahoo.com>> wrote:
>>>> +1 on the general idea to make the reloadability customizable per plugin.
>>>> 
>>>> However, I think it'd be more simple, cleaner and intuitive to not tie it to whether or not a plugin is used both as a global and remap plugin.
>>>> 
>>>> In other words, approach (1) below but, instead of calling it "@global", we could add a param which says "@reloadable=false" (the default value for "@reloadable" can be "true").
>>>> 
>>>> The same param can then be used, when we eventually add relodability to global plugins as well.
>>>> 
>>>> Thoughts?
>>>> 
>>>> 
>>>> 
>>>> 
>>>> On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <solidwallofcode@verizonmedia.com <ma...@verizonmedia.com>> wrote:
>>>> 
>>>> 
>>>> As part of the ATS 9 upgrade, a feature was added so that remap plugins could have their DSO reloaded. This means not just the configuration, but the implementation itself. While very useful, this has some unfortunate side effects with plugins that are used in both a global and remap context. To alleviate this, a configuration variable as added to disable the feature.
>>>> 
>>>> Although reasonable, this is a rather heavy handed way to deal with the problem. What would be better is the ability to reload the DSO or not on a per remap plugin basis. I have a few ways this could be done:
>>>> 
>>>> 1) Add the keyword "@global" to "remap.config". This would behave exactly as "@plugin" except it would prohibit reloading of the DSO for that plugin.
>>>> 
>>>> 2) Have the remap reload configuration check to see if the plugin is also a global plugin and disable remap DSO reload for that plugin.
>>>> 
>>>> 3) Add a flag to the global plugin registry information which can be set during TSPluginInit which disables DSO reloading for that plugin, should it occur in "remap.config". This is similar to (2) but requires a  plugin to prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and would only be valid when called from TSPluginInit.
>>>> 
>>>> 4) As (3), except the flag is set by default and must be cleared to enable DSO reloading in "remap.config".
>>>> 
>>>> I'm willing to see if I can make this work, but I would like to have some feedback on the preferred approach first.

Re: Remap plugin DSO reloading - enabling and disabling

[Alan Carroll <so...@verizonmedia.com>]

I think you'll have to wait until we've gone YAML, then it would be easy to
add an option per plugin to do that.

Re: Remap plugin DSO reloading - enabling and disabling

[Sudheer Vinukonda <su...@yahoo.com>]

The only concern I’ve with an API solution (options 3, 4) is exactly that. What if a user wants to also not reload (some of the) built-in plugins? Without that, it feels incomplete and inconsistent to me. A config driven approach can address that.

In any case, I think it comes down to the degree of how optional, we want to make the behavior. I’d be okay with the consensus, but IMHO, we should allow *any* plugin to be non-reloadable if a user chooses to. It should not be limited to only plugins that are either used in a mixed mode or only the plugins that a user has code/control over. 

Apologize, if I’m missing something, but I’m yet to hear any convincing arguments as to *why* this feature should not be optional. Again, I’d be totally okay with a consensus on this.

In fact, if we all agree that this should NOT be optional at all, then I’d much rather prefer to make the behavior consistent for all plugins (built-in and custom) - ie all or none - if it’s enabled, reloadable applies to all plugins or disabled to all plugins.

Sorry Alan, Leif - Don’t mean to drag the debate on, but couldn’t help raise the concern. Totally trust your judgement and will be fine with what you suggest.


> On May 9, 2020, at 4:19 PM, Leif Hedstrom <zw...@apache.org> wrote:
> 
> 
> 
> 
>>> On May 9, 2020, at 14:53, Alan Carroll <so...@verizonmedia.com> wrote:
>>> 
>> 
>> Having been on the other side now, I have to disagree it's a "marginal code change". I will need to do some significant restructuring to make it work for TxnBox. The biggest issue is that it's a surprise that static variables don't work as expected anymore. Certainly it can be dealt with in ATS 9, but it's a bit trickier for code that has to run in ATS 9 and a previous version. Not being able to use non-cost class statics has a big effect on my coding and design.
> 
> Ah, it’s the static variable stuff that I ran into as well ? That makes more sense, and I have no good answer for that, than either not using them, or what you suggested here. I opted for the former in cache_promote, but it was an easy change with little disadvantage.
> 
> I thought (wrongly) that you were referring to the same problems originally reported by LI. Sorry.
> 
>> 
>> I think the basic issue comes down to - why do I have to restructure *my* code to make *your* feature work? It imposes a significant burden on me for no benefit to me, something you normally oppose. Option (1) also imposes a burden on anyone using it, because they must remember to put "@reloadable=false" every time the plugin is used.  We can turn it off globally, but as you note that's not a long term solution as it's effectively certain someone will want to run plugins that need the feature, and others that don't.
>> 
>> To me, the straight forward approach is to enable the plugin writer to make the choice for his specific plugin. He knows whether it's reloadable or not, he's the one who should decide whether to structure his code to support the feature or not. This avoids putting a burden on anyone except the plugin author. In addition, option (3) puts the burden only on those plugins that don't support reload, and it's a light burden - one extra API call. No changes to plugins that can reload are needed. No changes to "remap.config" are needed. It also means if the plugin is updated at some point, it can be shipped and installed with no
> 
> Yeh I’m fine with option 3 as well. Make it so #1. Question : do you know if any core plugins that must change ?
> 
> Cheers,
> 
> 
> — Leif 
> 
> 
>> configuration changes. I think (3) gives the closest approach to "it just works", while being flexible enough to handle all the use cases brought up, except differential reloading between rules, which I think is not something anyone would ever do. Even then, it could be implemented on top of (3) if necessary.
>> 
>> I understand why this feature is useful, I just want to be able to have my plugin opt out if I think that's best for my plugin, without forcing all other plugins to also opt out.
>> 
>>> On Fri, May 8, 2020 at 4:13 PM Sudheer Vinukonda <su...@yahoo.com> wrote:
>>> It’s not so much of a problem, but I just feel that this falls into a kind of feature that should not be forced in a one-way only mode to everyone. 
>>> 
>>> I’m not really convinced that this feature is useful to every user and even if so, like most features in ATS, don’t see *why* it should not be configurable (unless, making it so largely complicates things and is not worth the ROI). I think as long as there are ATS users that do not want to enable this behavior in prod, (at least not right away), it seems reasonable that we should try and allow that.
>>> 
>>> Just my 2c :)
>>> 
>>> 
>>>>> On May 8, 2020, at 1:42 PM, Leif Hedstrom <zw...@apache.org> wrote:
>>>>> 
>>>> 
>>>> 
>>>>> On May 8, 2020, at 2:29 PM, Alan Carroll <so...@verizonmedia.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>>> On Fri, May 8, 2020 at 3:17 PM Leif Hedstrom <zw...@apache.org> wrote:
>>>>>> 
>>>>>> It should get reloaded for the second, not for the first. As far as I understand, this is fine, the Gancho made the code such that as long as something uses some version of a plugin, it will be kept forever.
>>>>>> 
>>>>> 
>>>>> I am dubious of that claim - I don't remember anything like that. We should ask Gancho, but IIRC the table of plugins for the remap doesn't track per rule, so for a given configuration and plugin, there is only one version of the plugin DSO loaded. 
>>>> 
>>>> It’ll require some changes to the core, I’m just saying there’s nothing technical that would prevent two remaps to point to differently loaded .so’s. Multiple versions of the plugin can live simultaneously already, simply by the fact that remap structure is ref-counted and some request can linger for hours or days.
>>>> 
>>>> Now, I guess I really don’t care much, as long as all the core plugins that supports both remap and global don’t limit how we use them (which it seemed at least option #2 would do, possibly #3 and #4 too?). Or, if you can tell me which plugin is having a problem here, I can look into fixing that problem (likely it’s my fault anyways).
>>>> 
>>>> I still don’t fully understand the problem that anyone has here; As Gancho explained when the setting was added, it’s a marginal code change to fix the behavior in plugins that do have problems. We also added the “global” user-data slot table, to make it easier to share data between different plugins (such that you don’t lose the connection between the once loaded global instance, and the reloadable remap instances).  The setting was added as “transitional” aid, making it easier to upgrade to 9.0.x without having to fix internal plugins immediately.
>>>> 
>>>> — Leif
>>>> 

Re: Remap plugin DSO reloading - enabling and disabling

[Leif Hedstrom <zw...@apache.org>]

> On May 9, 2020, at 14:53, Alan Carroll <so...@verizonmedia.com> wrote:
> 
> 
> Having been on the other side now, I have to disagree it's a "marginal code change". I will need to do some significant restructuring to make it work for TxnBox. The biggest issue is that it's a surprise that static variables don't work as expected anymore. Certainly it can be dealt with in ATS 9, but it's a bit trickier for code that has to run in ATS 9 and a previous version. Not being able to use non-cost class statics has a big effect on my coding and design.

Ah, it’s the static variable stuff that I ran into as well ? That makes more sense, and I have no good answer for that, than either not using them, or what you suggested here. I opted for the former in cache_promote, but it was an easy change with little disadvantage.

I thought (wrongly) that you were referring to the same problems originally reported by LI. Sorry.

> 
> I think the basic issue comes down to - why do I have to restructure *my* code to make *your* feature work? It imposes a significant burden on me for no benefit to me, something you normally oppose. Option (1) also imposes a burden on anyone using it, because they must remember to put "@reloadable=false" every time the plugin is used.  We can turn it off globally, but as you note that's not a long term solution as it's effectively certain someone will want to run plugins that need the feature, and others that don't.
> 
> To me, the straight forward approach is to enable the plugin writer to make the choice for his specific plugin. He knows whether it's reloadable or not, he's the one who should decide whether to structure his code to support the feature or not. This avoids putting a burden on anyone except the plugin author. In addition, option (3) puts the burden only on those plugins that don't support reload, and it's a light burden - one extra API call. No changes to plugins that can reload are needed. No changes to "remap.config" are needed. It also means if the plugin is updated at some point, it can be shipped and installed with no

Yeh I’m fine with option 3 as well. Make it so #1. Question : do you know if any core plugins that must change ?

Cheers,


— Leif 


> configuration changes. I think (3) gives the closest approach to "it just works", while being flexible enough to handle all the use cases brought up, except differential reloading between rules, which I think is not something anyone would ever do. Even then, it could be implemented on top of (3) if necessary.
> 
> I understand why this feature is useful, I just want to be able to have my plugin opt out if I think that's best for my plugin, without forcing all other plugins to also opt out.
> 
>> On Fri, May 8, 2020 at 4:13 PM Sudheer Vinukonda <su...@yahoo.com> wrote:
>> It’s not so much of a problem, but I just feel that this falls into a kind of feature that should not be forced in a one-way only mode to everyone. 
>> 
>> I’m not really convinced that this feature is useful to every user and even if so, like most features in ATS, don’t see *why* it should not be configurable (unless, making it so largely complicates things and is not worth the ROI). I think as long as there are ATS users that do not want to enable this behavior in prod, (at least not right away), it seems reasonable that we should try and allow that.
>> 
>> Just my 2c :)
>> 
>> 
>>>> On May 8, 2020, at 1:42 PM, Leif Hedstrom <zw...@apache.org> wrote:
>>>> 
>>> 
>>> 
>>>> On May 8, 2020, at 2:29 PM, Alan Carroll <so...@verizonmedia.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Fri, May 8, 2020 at 3:17 PM Leif Hedstrom <zw...@apache.org> wrote:
>>>>> 
>>>>> It should get reloaded for the second, not for the first. As far as I understand, this is fine, the Gancho made the code such that as long as something uses some version of a plugin, it will be kept forever.
>>>>> 
>>>> 
>>>> I am dubious of that claim - I don't remember anything like that. We should ask Gancho, but IIRC the table of plugins for the remap doesn't track per rule, so for a given configuration and plugin, there is only one version of the plugin DSO loaded. 
>>> 
>>> It’ll require some changes to the core, I’m just saying there’s nothing technical that would prevent two remaps to point to differently loaded .so’s. Multiple versions of the plugin can live simultaneously already, simply by the fact that remap structure is ref-counted and some request can linger for hours or days.
>>> 
>>> Now, I guess I really don’t care much, as long as all the core plugins that supports both remap and global don’t limit how we use them (which it seemed at least option #2 would do, possibly #3 and #4 too?). Or, if you can tell me which plugin is having a problem here, I can look into fixing that problem (likely it’s my fault anyways).
>>> 
>>> I still don’t fully understand the problem that anyone has here; As Gancho explained when the setting was added, it’s a marginal code change to fix the behavior in plugins that do have problems. We also added the “global” user-data slot table, to make it easier to share data between different plugins (such that you don’t lose the connection between the once loaded global instance, and the reloadable remap instances).  The setting was added as “transitional” aid, making it easier to upgrade to 9.0.x without having to fix internal plugins immediately.
>>> 
>>> — Leif
>>> 

Re: Remap plugin DSO reloading - enabling and disabling

[Alan Carroll <so...@verizonmedia.com>]

Having been on the other side now, I have to disagree it's a "marginal code
change". I will need to do some significant restructuring to make it work
for TxnBox. The biggest issue is that it's a surprise that static variables
don't work as expected anymore. Certainly it can be dealt with in ATS 9,
but it's a bit trickier for code that has to run in ATS 9 and a previous
version. Not being able to use non-cost class statics has a big effect on
my coding and design.

I think the basic issue comes down to - why do I have to restructure *my*
code to make *your* feature work? It imposes a significant burden on me for
no benefit to me, something you normally oppose. Option (1) also imposes a
burden on anyone using it, because they must remember to put
"@reloadable=false" every time the plugin is used.  We can turn it off
globally, but as you note that's not a long term solution as it's
effectively certain someone will want to run plugins that need the feature,
and others that don't.

To me, the straight forward approach is to enable the plugin writer to make
the choice for his specific plugin. He knows whether it's reloadable or
not, he's the one who should decide whether to structure his code to
support the feature or not. This avoids putting a burden on anyone except
the plugin author. In addition, option (3) puts the burden only on those
plugins that don't support reload, and it's a light burden - one extra API
call. No changes to plugins that can reload are needed. No changes to
"remap.config" are needed. It also means if the plugin is updated at some
point, it can be shipped and installed with no configuration changes. I
think (3) gives the closest approach to "it just works", while being
flexible enough to handle all the use cases brought up, except differential
reloading between rules, which I think is not something anyone would ever
do. Even then, it could be implemented on top of (3) if necessary.

I understand why this feature is useful, I just want to be able to have my
plugin opt out if I think that's best for my plugin, without forcing all
other plugins to also opt out.

On Fri, May 8, 2020 at 4:13 PM Sudheer Vinukonda <su...@yahoo.com>
wrote:

> It’s not so much of a problem, but I just feel that this falls into a kind
> of feature that should not be forced in a one-way only mode to everyone.
>
> I’m not really convinced that this feature is useful to every user and
> even if so, like most features in ATS, don’t see *why* it should not be
> configurable (unless, making it so largely complicates things and is not
> worth the ROI). I think as long as there are ATS users that do not want to
> enable this behavior in prod, (at least not right away), it seems
> reasonable that we should try and allow that.
>
> Just my 2c :)
>
>
> On May 8, 2020, at 1:42 PM, Leif Hedstrom <zw...@apache.org> wrote:
>
> 
>
> On May 8, 2020, at 2:29 PM, Alan Carroll <so...@verizonmedia.com>
> wrote:
>
>
>
> On Fri, May 8, 2020 at 3:17 PM Leif Hedstrom <zw...@apache.org> wrote:
>
>>
>> It should get reloaded for the second, not for the first. As far as I
>> understand, this is fine, the Gancho made the code such that as long as
>> something uses some version of a plugin, it will be kept forever.
>>
>>
> I am dubious of that claim - I don't remember anything like that. We
> should ask Gancho, but IIRC the table of plugins for the remap doesn't
> track per rule, so for a given configuration and plugin, there is only one
> version of the plugin DSO loaded.
>
>
> It’ll require some changes to the core, I’m just saying there’s nothing
> technical that would prevent two remaps to point to differently loaded
> .so’s. Multiple versions of the plugin can live simultaneously already,
> simply by the fact that remap structure is ref-counted and some request can
> linger for hours or days.
>
> Now, I guess I really don’t care much, as long as all the core plugins
> that supports both remap and global don’t limit how we use them (which it
> seemed at least option #2 would do, possibly #3 and #4 too?). Or, if you
> can tell me which plugin is having a problem here, I can look into fixing
> that problem (likely it’s my fault anyways).
>
> I still don’t fully understand the problem that anyone has here; As Gancho
> explained when the setting was added, it’s a marginal code change to fix
> the behavior in plugins that do have problems. We also added the “global”
> user-data slot table, to make it easier to share data between different
> plugins (such that you don’t lose the connection between the once loaded
> global instance, and the reloadable remap instances).  The setting was
> added as “transitional” aid, making it easier to upgrade to 9.0.x without
> having to fix internal plugins immediately.
>
> — Leif
>
>

Re: Remap plugin DSO reloading - enabling and disabling

[Sudheer Vinukonda <su...@yahoo.com>]

It’s not so much of a problem, but I just feel that this falls into a kind of feature that should not be forced in a one-way only mode to everyone. 

I’m not really convinced that this feature is useful to every user and even if so, like most features in ATS, don’t see *why* it should not be configurable (unless, making it so largely complicates things and is not worth the ROI). I think as long as there are ATS users that do not want to enable this behavior in prod, (at least not right away), it seems reasonable that we should try and allow that.

Just my 2c :)


> On May 8, 2020, at 1:42 PM, Leif Hedstrom <zw...@apache.org> wrote:
> 
> 
> 
>> On May 8, 2020, at 2:29 PM, Alan Carroll <so...@verizonmedia.com> wrote:
>> 
>> 
>> 
>>> On Fri, May 8, 2020 at 3:17 PM Leif Hedstrom <zw...@apache.org> wrote:
>>> 
>>> It should get reloaded for the second, not for the first. As far as I understand, this is fine, the Gancho made the code such that as long as something uses some version of a plugin, it will be kept forever.
>>> 
>> 
>> I am dubious of that claim - I don't remember anything like that. We should ask Gancho, but IIRC the table of plugins for the remap doesn't track per rule, so for a given configuration and plugin, there is only one version of the plugin DSO loaded. 
> 
> It’ll require some changes to the core, I’m just saying there’s nothing technical that would prevent two remaps to point to differently loaded .so’s. Multiple versions of the plugin can live simultaneously already, simply by the fact that remap structure is ref-counted and some request can linger for hours or days.
> 
> Now, I guess I really don’t care much, as long as all the core plugins that supports both remap and global don’t limit how we use them (which it seemed at least option #2 would do, possibly #3 and #4 too?). Or, if you can tell me which plugin is having a problem here, I can look into fixing that problem (likely it’s my fault anyways).
> 
> I still don’t fully understand the problem that anyone has here; As Gancho explained when the setting was added, it’s a marginal code change to fix the behavior in plugins that do have problems. We also added the “global” user-data slot table, to make it easier to share data between different plugins (such that you don’t lose the connection between the once loaded global instance, and the reloadable remap instances).  The setting was added as “transitional” aid, making it easier to upgrade to 9.0.x without having to fix internal plugins immediately.
> 
> — Leif
> 

Re: Remap plugin DSO reloading - enabling and disabling

[Leif Hedstrom <zw...@apache.org>]

> On May 8, 2020, at 2:29 PM, Alan Carroll <so...@verizonmedia.com> wrote:
> 
> 
> 
> On Fri, May 8, 2020 at 3:17 PM Leif Hedstrom <zwoop@apache.org <ma...@apache.org>> wrote:
> 
> It should get reloaded for the second, not for the first. As far as I understand, this is fine, the Gancho made the code such that as long as something uses some version of a plugin, it will be kept forever.
> 
> 
> I am dubious of that claim - I don't remember anything like that. We should ask Gancho, but IIRC the table of plugins for the remap doesn't track per rule, so for a given configuration and plugin, there is only one version of the plugin DSO loaded. 

It’ll require some changes to the core, I’m just saying there’s nothing technical that would prevent two remaps to point to differently loaded .so’s. Multiple versions of the plugin can live simultaneously already, simply by the fact that remap structure is ref-counted and some request can linger for hours or days.

Now, I guess I really don’t care much, as long as all the core plugins that supports both remap and global don’t limit how we use them (which it seemed at least option #2 would do, possibly #3 and #4 too?). Or, if you can tell me which plugin is having a problem here, I can look into fixing that problem (likely it’s my fault anyways).

I still don’t fully understand the problem that anyone has here; As Gancho explained when the setting was added, it’s a marginal code change to fix the behavior in plugins that do have problems. We also added the “global” user-data slot table, to make it easier to share data between different plugins (such that you don’t lose the connection between the once loaded global instance, and the reloadable remap instances).  The setting was added as “transitional” aid, making it easier to upgrade to 9.0.x without having to fix internal plugins immediately.

— Leif

Re: Remap plugin DSO reloading - enabling and disabling

[Alan Carroll <so...@verizonmedia.com>]

On Fri, May 8, 2020 at 3:17 PM Leif Hedstrom <zw...@apache.org> wrote:

>
> It should get reloaded for the second, not for the first. As far as I
> understand, this is fine, the Gancho made the code such that as long as
> something uses some version of a plugin, it will be kept forever.
>
>
I am dubious of that claim - I don't remember anything like that. We should
ask Gancho, but IIRC the table of plugins for the remap doesn't track per
rule, so for a given configuration and plugin, there is only one version of
the plugin DSO loaded.

Re: Remap plugin DSO reloading - enabling and disabling

[Leif Hedstrom <zw...@apache.org>]

> On May 8, 2020, at 12:29 PM, Alan Carroll <so...@verizonmedia.com> wrote:
> 
> Consider a situation with option (1) with two remap rules:
> 
> map http://example.one <http://example.one/> http://example.one <http://example.one/> @plugin=txn_box.so @reloadable=false blah blah blah
> map http://example.two <http://example.two/> http://example.two <http://example.two/> @plugin=txn_box.so @reloadable=true blah blah
> 
> Does that DSO get reloaded on a reload of "remap.config"?


It should get reloaded for the second, not for the first. As far as I understand, this is fine, the Gancho made the code such that as long as something uses some version of a plugin, it will be kept forever.

— Leif



> 
> 
> On Fri, May 8, 2020 at 9:58 AM Sudheer Vinukonda <sudheervinukonda@yahoo.com <ma...@yahoo.com>> wrote:
> Ah, true. I get the misunderstanding now. Yeah, I don’t mean to have reloadable flexibility per remap line either, but just per “(remap)plugin”.
> 
> And the only point I was trying to make was to let that the flexibility be determined by the user and not implicitly by the fact that a plugin was used in mixed mode. And yeah sorry, I totally missed the problem with making it a remap level param instead of a plugin level param. So, I still prefer your approach 1, except it’d be clearer if it’s named something more obvious indicating non-reload ability than “@global” (but, naming is hard and I can’t think of a short/succinct better name :()
> 
> 
> 
>> On May 8, 2020, at 7:33 AM, Alan Carroll <solidwallofcode@verizonmedia.com <ma...@verizonmedia.com>> wrote:
>> 
>> 
>> Sudheer, I understand the point you are making, I just consider it irrelevant. Let me give Leif an example to illustrate why - TxnBox. It shares data between the global and remap configurations at run time via static variables. If you enable remap DSO reloading for TxnBox, it will crash on the first transaction that hits a remap rule. It doesn't matter if it's actually been reloaded or not. However your organization does plugin updates, TxnBox will still crash in that situation. Even in your example, Sudheer, there's no _choice_ about whether a particular plugin can be DSO reloaded, it's a result of the implementation. As you yourself write, you can't enable it for those plugins without changing the code. No configuration cleverness will get around that.
>> 
>> For plugins that do support DSO reloading, the enablement is still per plugin, not per remap rule. Moreover, if we went with option (3) it would be simple to have to plugin support a configuration / load time option to enable or disable DSO reloading. In general, if the plugin can be DSO reloaded, it's unclear why it shouldn't be except in unusual circumstances which are depending on the plugin implementation.
>> 
>> For Sudheer, I remain unclear on what exact flexibility you want, given the constraints created by a specific plugin's implementation. I've re-read your note and AFAICT it assumes doing DSO reload or not *per plugin*, which is also my point. I dislike (1) because it makes no sense to me to have this change between remap rules for a specific plugin. I think it's better to have the plugin decide if that's possible and, if needed, provide configuration to disable it if needed. Speaking specifically for TxnBox, I must forbid you from enabling DSO reloading. Even in your case, it might be reasonable to have this for plugins that you have not yet updated (which is actually the case with TxnBox - I'm limited by a requirement for ATS 7 compatibility, so I can't change that feature at the current time).
>> 
>> On Thu, May 7, 2020 at 10:11 PM Leif Hedstrom <zwoop@apache.org <ma...@apache.org>> wrote:
>> 
>> 
>>> On May 7, 2020, at 8:12 PM, Alan Carroll <solidwallofcode@verizonmedia.com <ma...@verizonmedia.com>> wrote:
>>> 
>>> Leif;
>>> 
>>> If the plugin can be global or remap but not both, I don't see why (2) limits anything. The entire issue is irrelevant for such plugins, because the situation of reloading the remap DSO but not the global cannot occur, In fact, option (3) or (4) would enable detecting this and issuing a warning.
>> 
>> Ah yes, good point. However, still the same problem, one can very much want to use say header_rewrite as both global and remap plugin at the same time, and be fine with the fact that it doesn’t reload as a “global”, but you want it to reload as a remap. We use that plugin in this way for example. 
>> 
>> I still feel that option 2) is a bad option, but I’m ok with the others (still with a preference towards #1). I think a finer granular control mechanism here is a good idea.
>> 
>> I’d also be curious to hear which of the core plugins are having problems here, in most cases, there’s a no dependency between the global instantiation, and the per remap instantiation. Sudheer and LinkedIn have many internal plugins that do experience this problem however, so I’m guessing that maybe you have similar custom internal plugins?
>> 
>> Cheers,
>> 
>> — Leif
>> 
>>> 
>>> Approach (1) was my first thought, but I think the problem there is whether the plugin can work as a global and a reloadable remap is a property of the plugin implementation, not any particular remap rule. That is, for a specific plugin, there's really no choice about whether to use "@plugin" or "@global" - the configuration must get it right or the plugin crashes. Every time. Every rule. It is for this reason I disagree with Leif's view the user should decide. The user's opinion is irrelevant - the plugin works in this mode, or it doesn't. And as our friends at LinkedIn discovered, some rather basic C++ decisions (such as using static variables) will prevent a plugin from working in this mode. On the other hand, if the plugin uses the "User Args" feature then it can work, in which case what's the point of disabling the DSO reload? Unless the plugin implementor is concerned about code skew between the global and remap versions, which again the user is not qualified to decide.
>>> 
>>> My personal preference is (3), but I suspect after mysterious crashes with plugins, we will have been happier with (4).
>>> 
>>> On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda <sudheervinukonda@yahoo.com <ma...@yahoo.com>> wrote:
>>> +1 on the general idea to make the reloadability customizable per plugin.
>>> 
>>> However, I think it'd be more simple, cleaner and intuitive to not tie it to whether or not a plugin is used both as a global and remap plugin.
>>> 
>>> In other words, approach (1) below but, instead of calling it "@global", we could add a param which says "@reloadable=false" (the default value for "@reloadable" can be "true").
>>> 
>>> The same param can then be used, when we eventually add relodability to global plugins as well.
>>> 
>>> Thoughts?
>>> 
>>> 
>>> 
>>> 
>>> On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <solidwallofcode@verizonmedia.com <ma...@verizonmedia.com>> wrote:
>>> 
>>> 
>>> As part of the ATS 9 upgrade, a feature was added so that remap plugins could have their DSO reloaded. This means not just the configuration, but the implementation itself. While very useful, this has some unfortunate side effects with plugins that are used in both a global and remap context. To alleviate this, a configuration variable as added to disable the feature.
>>> 
>>> Although reasonable, this is a rather heavy handed way to deal with the problem. What would be better is the ability to reload the DSO or not on a per remap plugin basis. I have a few ways this could be done:
>>> 
>>> 1) Add the keyword "@global" to "remap.config". This would behave exactly as "@plugin" except it would prohibit reloading of the DSO for that plugin.
>>> 
>>> 2) Have the remap reload configuration check to see if the plugin is also a global plugin and disable remap DSO reload for that plugin.
>>> 
>>> 3) Add a flag to the global plugin registry information which can be set during TSPluginInit which disables DSO reloading for that plugin, should it occur in "remap.config". This is similar to (2) but requires a  plugin to prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and would only be valid when called from TSPluginInit.
>>> 
>>> 4) As (3), except the flag is set by default and must be cleared to enable DSO reloading in "remap.config".
>>> 
>>> I'm willing to see if I can make this work, but I would like to have some feedback on the preferred approach first.
>>> 
>> 

Re: Remap plugin DSO reloading - enabling and disabling

[Alan Carroll <so...@verizonmedia.com>]

Consider a situation with option (1) with two remap rules:

map http://example.one http://example.one @plugin=txn_box.so @reloadable=false
blah blah blah
map http://example.two http://example.two @plugin=txn_box.so @reloadable=true
blah blah

Does that DSO get reloaded on a reload of "remap.config"?


On Fri, May 8, 2020 at 9:58 AM Sudheer Vinukonda <su...@yahoo.com>
wrote:

> Ah, true. I get the misunderstanding now. Yeah, I don’t mean to have
> reloadable flexibility per remap line either, but just per “(remap)plugin”.
>
> And the only point I was trying to make was to let that the flexibility be
> determined by the user and not implicitly by the fact that a plugin was
> used in mixed mode. And yeah sorry, I totally missed the problem with
> making it a remap level param instead of a plugin level param. So, I still
> prefer your approach 1, except it’d be clearer if it’s named something more
> obvious indicating non-reload ability than “@global” (but, naming is hard
> and I can’t think of a short/succinct better name :()
>
>
>
> On May 8, 2020, at 7:33 AM, Alan Carroll <so...@verizonmedia.com>
> wrote:
>
> 
> Sudheer, I understand the point you are making, I just consider it
> irrelevant. Let me give Leif an example to illustrate why - TxnBox. It
> shares data between the global and remap configurations at run time via
> static variables. If you enable remap DSO reloading for TxnBox, it will
> crash on the first transaction that hits a remap rule. It doesn't matter if
> it's actually been reloaded or not. However your organization does plugin
> updates, TxnBox will still crash in that situation. Even in your example,
> Sudheer, there's no _choice_ about whether a particular plugin can be DSO
> reloaded, it's a result of the implementation. As you yourself write, you
> can't enable it for those plugins without changing the code. No
> configuration cleverness will get around that.
>
> For plugins that do support DSO reloading, the enablement is still per
> plugin, not per remap rule. Moreover, if we went with option (3) it would
> be simple to have to plugin support a configuration / load time option to
> enable or disable DSO reloading. In general, if the plugin can be DSO
> reloaded, it's unclear why it shouldn't be except in unusual circumstances
> which are depending on the plugin implementation.
>
> For Sudheer, I remain unclear on what exact flexibility you want, given
> the constraints created by a specific plugin's implementation. I've re-read
> your note and AFAICT it assumes doing DSO reload or not *per plugin*, which
> is also my point. I dislike (1) because it makes no sense to me to have
> this change between remap rules for a specific plugin. I think it's better
> to have the plugin decide if that's possible and, if needed, provide
> configuration to disable it if needed. Speaking specifically for TxnBox, I
> must forbid you from enabling DSO reloading. Even in your case, it might be
> reasonable to have this for plugins that you have not yet updated (which is
> actually the case with TxnBox - I'm limited by a requirement for ATS 7
> compatibility, so I can't change that feature at the current time).
>
> On Thu, May 7, 2020 at 10:11 PM Leif Hedstrom <zw...@apache.org> wrote:
>
>>
>>
>> On May 7, 2020, at 8:12 PM, Alan Carroll <
>> solidwallofcode@verizonmedia.com> wrote:
>>
>> Leif;
>>
>> If the plugin can be global or remap but not both, I don't see why (2)
>> limits anything. The entire issue is irrelevant for such plugins, because
>> the situation of reloading the remap DSO but not the global cannot occur,
>> In fact, option (3) or (4) would enable detecting this and issuing a
>> warning.
>>
>>
>> Ah yes, good point. However, still the same problem, one can very much
>> want to use say header_rewrite as both global and remap plugin at the same
>> time, and be fine with the fact that it doesn’t reload as a “global”, but
>> you want it to reload as a remap. We use that plugin in this way for
>> example.
>>
>> I still feel that option 2) is a bad option, but I’m ok with the others
>> (still with a preference towards #1). I think a finer granular control
>> mechanism here is a good idea.
>>
>> I’d also be curious to hear which of the core plugins are having problems
>> here, in most cases, there’s a no dependency between the global
>> instantiation, and the per remap instantiation. Sudheer and LinkedIn have
>> many internal plugins that do experience this problem however, so I’m
>> guessing that maybe you have similar custom internal plugins?
>>
>> Cheers,
>>
>> — Leif
>>
>>
>> Approach (1) was my first thought, but I think the problem there is
>> whether the plugin can work as a global and a reloadable remap is a
>> property of the plugin implementation, not any particular remap rule. That
>> is, for a specific plugin, there's really no choice about whether to use
>> "@plugin" or "@global" - the configuration must get it right or the plugin
>> crashes. Every time. Every rule. It is for this reason I disagree with
>> Leif's view the user should decide. The user's opinion is irrelevant - the
>> plugin works in this mode, or it doesn't. And as our friends at LinkedIn
>> discovered, some rather basic C++ decisions (such as using static
>> variables) will prevent a plugin from working in this mode. On the other
>> hand, if the plugin uses the "User Args" feature then it can work, in which
>> case what's the point of disabling the DSO reload? Unless the plugin
>> implementor is concerned about code skew between the global and remap
>> versions, which again the user is not qualified to decide.
>>
>> My personal preference is (3), but I suspect after mysterious crashes
>> with plugins, we will have been happier with (4).
>>
>> On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda <
>> sudheervinukonda@yahoo.com> wrote:
>>
>>> +1 on the general idea to make the reloadability customizable per plugin.
>>>
>>> However, I think it'd be more simple, cleaner and intuitive to not tie
>>> it to whether or not a plugin is used both as a global and remap plugin.
>>>
>>> In other words, approach (1) below but, instead of calling it "@global",
>>> we could add a param which says "@reloadable=false" (the default value for
>>> "@reloadable" can be "true").
>>>
>>> The same param can then be used, when we eventually add relodability to
>>> global plugins as well.
>>>
>>> Thoughts?
>>>
>>>
>>>
>>>
>>> On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <
>>> solidwallofcode@verizonmedia.com> wrote:
>>>
>>>
>>> As part of the ATS 9 upgrade, a feature was added so that remap plugins
>>> could have their DSO reloaded. This means not just the configuration, but
>>> the implementation itself. While very useful, this has some
>>> unfortunate side effects with plugins that are used in both a global and
>>> remap context. To alleviate this, a configuration variable as added to
>>> disable the feature.
>>>
>>> Although reasonable, this is a rather heavy handed way to deal with the
>>> problem. What would be better is the ability to reload the DSO or not on a
>>> per remap plugin basis. I have a few ways this could be done:
>>>
>>> 1) Add the keyword "@global" to "remap.config". This would behave
>>> exactly as "@plugin" except it would prohibit reloading of the DSO for that
>>> plugin.
>>>
>>> 2) Have the remap reload configuration check to see if the plugin is
>>> also a global plugin and disable remap DSO reload for that plugin.
>>>
>>> 3) Add a flag to the global plugin registry information which can be set
>>> during TSPluginInit which disables DSO reloading for that plugin, should it
>>> occur in "remap.config". This is similar to (2) but requires a  plugin to
>>> prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and
>>> would only be valid when called from TSPluginInit.
>>>
>>> 4) As (3), except the flag is set by default and must be cleared to
>>> enable DSO reloading in "remap.config".
>>>
>>> I'm willing to see if I can make this work, but I would like to have
>>> some feedback on the preferred approach first.
>>>
>>>
>>

Re: Remap plugin DSO reloading - enabling and disabling

[Sudheer Vinukonda <su...@yahoo.com>]

Ah, true. I get the misunderstanding now. Yeah, I don’t mean to have reloadable flexibility per remap line either, but just per “(remap)plugin”.

And the only point I was trying to make was to let that the flexibility be determined by the user and not implicitly by the fact that a plugin was used in mixed mode. And yeah sorry, I totally missed the problem with making it a remap level param instead of a plugin level param. So, I still prefer your approach 1, except it’d be clearer if it’s named something more obvious indicating non-reload ability than “@global” (but, naming is hard and I can’t think of a short/succinct better name :()



> On May 8, 2020, at 7:33 AM, Alan Carroll <so...@verizonmedia.com> wrote:
> 
> 
> Sudheer, I understand the point you are making, I just consider it irrelevant. Let me give Leif an example to illustrate why - TxnBox. It shares data between the global and remap configurations at run time via static variables. If you enable remap DSO reloading for TxnBox, it will crash on the first transaction that hits a remap rule. It doesn't matter if it's actually been reloaded or not. However your organization does plugin updates, TxnBox will still crash in that situation. Even in your example, Sudheer, there's no _choice_ about whether a particular plugin can be DSO reloaded, it's a result of the implementation. As you yourself write, you can't enable it for those plugins without changing the code. No configuration cleverness will get around that.
> 
> For plugins that do support DSO reloading, the enablement is still per plugin, not per remap rule. Moreover, if we went with option (3) it would be simple to have to plugin support a configuration / load time option to enable or disable DSO reloading. In general, if the plugin can be DSO reloaded, it's unclear why it shouldn't be except in unusual circumstances which are depending on the plugin implementation.
> 
> For Sudheer, I remain unclear on what exact flexibility you want, given the constraints created by a specific plugin's implementation. I've re-read your note and AFAICT it assumes doing DSO reload or not *per plugin*, which is also my point. I dislike (1) because it makes no sense to me to have this change between remap rules for a specific plugin. I think it's better to have the plugin decide if that's possible and, if needed, provide configuration to disable it if needed. Speaking specifically for TxnBox, I must forbid you from enabling DSO reloading. Even in your case, it might be reasonable to have this for plugins that you have not yet updated (which is actually the case with TxnBox - I'm limited by a requirement for ATS 7 compatibility, so I can't change that feature at the current time).
> 
>> On Thu, May 7, 2020 at 10:11 PM Leif Hedstrom <zw...@apache.org> wrote:
>> 
>> 
>>> On May 7, 2020, at 8:12 PM, Alan Carroll <so...@verizonmedia.com> wrote:
>>> 
>>> Leif;
>>> 
>>> If the plugin can be global or remap but not both, I don't see why (2) limits anything. The entire issue is irrelevant for such plugins, because the situation of reloading the remap DSO but not the global cannot occur, In fact, option (3) or (4) would enable detecting this and issuing a warning.
>> 
>> Ah yes, good point. However, still the same problem, one can very much want to use say header_rewrite as both global and remap plugin at the same time, and be fine with the fact that it doesn’t reload as a “global”, but you want it to reload as a remap. We use that plugin in this way for example. 
>> 
>> I still feel that option 2) is a bad option, but I’m ok with the others (still with a preference towards #1). I think a finer granular control mechanism here is a good idea.
>> 
>> I’d also be curious to hear which of the core plugins are having problems here, in most cases, there’s a no dependency between the global instantiation, and the per remap instantiation. Sudheer and LinkedIn have many internal plugins that do experience this problem however, so I’m guessing that maybe you have similar custom internal plugins?
>> 
>> Cheers,
>> 
>> — Leif
>> 
>>> 
>>> Approach (1) was my first thought, but I think the problem there is whether the plugin can work as a global and a reloadable remap is a property of the plugin implementation, not any particular remap rule. That is, for a specific plugin, there's really no choice about whether to use "@plugin" or "@global" - the configuration must get it right or the plugin crashes. Every time. Every rule. It is for this reason I disagree with Leif's view the user should decide. The user's opinion is irrelevant - the plugin works in this mode, or it doesn't. And as our friends at LinkedIn discovered, some rather basic C++ decisions (such as using static variables) will prevent a plugin from working in this mode. On the other hand, if the plugin uses the "User Args" feature then it can work, in which case what's the point of disabling the DSO reload? Unless the plugin implementor is concerned about code skew between the global and remap versions, which again the user is not qualified to decide.
>>> 
>>> My personal preference is (3), but I suspect after mysterious crashes with plugins, we will have been happier with (4).
>>> 
>>>> On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda <su...@yahoo.com> wrote:
>>>> +1 on the general idea to make the reloadability customizable per plugin.
>>>> 
>>>> However, I think it'd be more simple, cleaner and intuitive to not tie it to whether or not a plugin is used both as a global and remap plugin.
>>>> 
>>>> In other words, approach (1) below but, instead of calling it "@global", we could add a param which says "@reloadable=false" (the default value for "@reloadable" can be "true").
>>>> 
>>>> The same param can then be used, when we eventually add relodability to global plugins as well.
>>>> 
>>>> Thoughts?
>>>> 
>>>> 
>>>> 
>>>> 
>>>> On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <so...@verizonmedia.com> wrote:
>>>> 
>>>> 
>>>> As part of the ATS 9 upgrade, a feature was added so that remap plugins could have their DSO reloaded. This means not just the configuration, but the implementation itself. While very useful, this has some unfortunate side effects with plugins that are used in both a global and remap context. To alleviate this, a configuration variable as added to disable the feature.
>>>> 
>>>> Although reasonable, this is a rather heavy handed way to deal with the problem. What would be better is the ability to reload the DSO or not on a per remap plugin basis. I have a few ways this could be done:
>>>> 
>>>> 1) Add the keyword "@global" to "remap.config". This would behave exactly as "@plugin" except it would prohibit reloading of the DSO for that plugin.
>>>> 
>>>> 2) Have the remap reload configuration check to see if the plugin is also a global plugin and disable remap DSO reload for that plugin.
>>>> 
>>>> 3) Add a flag to the global plugin registry information which can be set during TSPluginInit which disables DSO reloading for that plugin, should it occur in "remap.config". This is similar to (2) but requires a  plugin to prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and would only be valid when called from TSPluginInit.
>>>> 
>>>> 4) As (3), except the flag is set by default and must be cleared to enable DSO reloading in "remap.config".
>>>> 
>>>> I'm willing to see if I can make this work, but I would like to have some feedback on the preferred approach first.
>>>> 
>> 

Re: Remap plugin DSO reloading - enabling and disabling

[Alan Carroll <so...@verizonmedia.com>]

Sudheer, I understand the point you are making, I just consider it
irrelevant. Let me give Leif an example to illustrate why - TxnBox. It
shares data between the global and remap configurations at run time via
static variables. If you enable remap DSO reloading for TxnBox, it will
crash on the first transaction that hits a remap rule. It doesn't matter if
it's actually been reloaded or not. However your organization does plugin
updates, TxnBox will still crash in that situation. Even in your example,
Sudheer, there's no _choice_ about whether a particular plugin can be DSO
reloaded, it's a result of the implementation. As you yourself write, you
can't enable it for those plugins without changing the code. No
configuration cleverness will get around that.

For plugins that do support DSO reloading, the enablement is still per
plugin, not per remap rule. Moreover, if we went with option (3) it would
be simple to have to plugin support a configuration / load time option to
enable or disable DSO reloading. In general, if the plugin can be DSO
reloaded, it's unclear why it shouldn't be except in unusual circumstances
which are depending on the plugin implementation.

For Sudheer, I remain unclear on what exact flexibility you want, given the
constraints created by a specific plugin's implementation. I've re-read
your note and AFAICT it assumes doing DSO reload or not *per plugin*, which
is also my point. I dislike (1) because it makes no sense to me to have
this change between remap rules for a specific plugin. I think it's better
to have the plugin decide if that's possible and, if needed, provide
configuration to disable it if needed. Speaking specifically for TxnBox, I
must forbid you from enabling DSO reloading. Even in your case, it might be
reasonable to have this for plugins that you have not yet updated (which is
actually the case with TxnBox - I'm limited by a requirement for ATS 7
compatibility, so I can't change that feature at the current time).

On Thu, May 7, 2020 at 10:11 PM Leif Hedstrom <zw...@apache.org> wrote:

>
>
> On May 7, 2020, at 8:12 PM, Alan Carroll <so...@verizonmedia.com>
> wrote:
>
> Leif;
>
> If the plugin can be global or remap but not both, I don't see why (2)
> limits anything. The entire issue is irrelevant for such plugins, because
> the situation of reloading the remap DSO but not the global cannot occur,
> In fact, option (3) or (4) would enable detecting this and issuing a
> warning.
>
>
> Ah yes, good point. However, still the same problem, one can very much
> want to use say header_rewrite as both global and remap plugin at the same
> time, and be fine with the fact that it doesn’t reload as a “global”, but
> you want it to reload as a remap. We use that plugin in this way for
> example.
>
> I still feel that option 2) is a bad option, but I’m ok with the others
> (still with a preference towards #1). I think a finer granular control
> mechanism here is a good idea.
>
> I’d also be curious to hear which of the core plugins are having problems
> here, in most cases, there’s a no dependency between the global
> instantiation, and the per remap instantiation. Sudheer and LinkedIn have
> many internal plugins that do experience this problem however, so I’m
> guessing that maybe you have similar custom internal plugins?
>
> Cheers,
>
> — Leif
>
>
> Approach (1) was my first thought, but I think the problem there is
> whether the plugin can work as a global and a reloadable remap is a
> property of the plugin implementation, not any particular remap rule. That
> is, for a specific plugin, there's really no choice about whether to use
> "@plugin" or "@global" - the configuration must get it right or the plugin
> crashes. Every time. Every rule. It is for this reason I disagree with
> Leif's view the user should decide. The user's opinion is irrelevant - the
> plugin works in this mode, or it doesn't. And as our friends at LinkedIn
> discovered, some rather basic C++ decisions (such as using static
> variables) will prevent a plugin from working in this mode. On the other
> hand, if the plugin uses the "User Args" feature then it can work, in which
> case what's the point of disabling the DSO reload? Unless the plugin
> implementor is concerned about code skew between the global and remap
> versions, which again the user is not qualified to decide.
>
> My personal preference is (3), but I suspect after mysterious crashes with
> plugins, we will have been happier with (4).
>
> On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda <
> sudheervinukonda@yahoo.com> wrote:
>
>> +1 on the general idea to make the reloadability customizable per plugin.
>>
>> However, I think it'd be more simple, cleaner and intuitive to not tie it
>> to whether or not a plugin is used both as a global and remap plugin.
>>
>> In other words, approach (1) below but, instead of calling it "@global",
>> we could add a param which says "@reloadable=false" (the default value for
>> "@reloadable" can be "true").
>>
>> The same param can then be used, when we eventually add relodability to
>> global plugins as well.
>>
>> Thoughts?
>>
>>
>>
>>
>> On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <
>> solidwallofcode@verizonmedia.com> wrote:
>>
>>
>> As part of the ATS 9 upgrade, a feature was added so that remap plugins
>> could have their DSO reloaded. This means not just the configuration, but
>> the implementation itself. While very useful, this has some
>> unfortunate side effects with plugins that are used in both a global and
>> remap context. To alleviate this, a configuration variable as added to
>> disable the feature.
>>
>> Although reasonable, this is a rather heavy handed way to deal with the
>> problem. What would be better is the ability to reload the DSO or not on a
>> per remap plugin basis. I have a few ways this could be done:
>>
>> 1) Add the keyword "@global" to "remap.config". This would behave exactly
>> as "@plugin" except it would prohibit reloading of the DSO for that plugin.
>>
>> 2) Have the remap reload configuration check to see if the plugin is also
>> a global plugin and disable remap DSO reload for that plugin.
>>
>> 3) Add a flag to the global plugin registry information which can be set
>> during TSPluginInit which disables DSO reloading for that plugin, should it
>> occur in "remap.config". This is similar to (2) but requires a  plugin to
>> prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and
>> would only be valid when called from TSPluginInit.
>>
>> 4) As (3), except the flag is set by default and must be cleared to
>> enable DSO reloading in "remap.config".
>>
>> I'm willing to see if I can make this work, but I would like to have some
>> feedback on the preferred approach first.
>>
>>
>

Re: Remap plugin DSO reloading - enabling and disabling

[Leif Hedstrom <zw...@apache.org>]

> On May 7, 2020, at 8:12 PM, Alan Carroll <so...@verizonmedia.com> wrote:
> 
> Leif;
> 
> If the plugin can be global or remap but not both, I don't see why (2) limits anything. The entire issue is irrelevant for such plugins, because the situation of reloading the remap DSO but not the global cannot occur, In fact, option (3) or (4) would enable detecting this and issuing a warning.

Ah yes, good point. However, still the same problem, one can very much want to use say header_rewrite as both global and remap plugin at the same time, and be fine with the fact that it doesn’t reload as a “global”, but you want it to reload as a remap. We use that plugin in this way for example. 

I still feel that option 2) is a bad option, but I’m ok with the others (still with a preference towards #1). I think a finer granular control mechanism here is a good idea.

I’d also be curious to hear which of the core plugins are having problems here, in most cases, there’s a no dependency between the global instantiation, and the per remap instantiation. Sudheer and LinkedIn have many internal plugins that do experience this problem however, so I’m guessing that maybe you have similar custom internal plugins?

Cheers,

— Leif

> 
> Approach (1) was my first thought, but I think the problem there is whether the plugin can work as a global and a reloadable remap is a property of the plugin implementation, not any particular remap rule. That is, for a specific plugin, there's really no choice about whether to use "@plugin" or "@global" - the configuration must get it right or the plugin crashes. Every time. Every rule. It is for this reason I disagree with Leif's view the user should decide. The user's opinion is irrelevant - the plugin works in this mode, or it doesn't. And as our friends at LinkedIn discovered, some rather basic C++ decisions (such as using static variables) will prevent a plugin from working in this mode. On the other hand, if the plugin uses the "User Args" feature then it can work, in which case what's the point of disabling the DSO reload? Unless the plugin implementor is concerned about code skew between the global and remap versions, which again the user is not qualified to decide.
> 
> My personal preference is (3), but I suspect after mysterious crashes with plugins, we will have been happier with (4).
> 
> On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda <sudheervinukonda@yahoo.com <ma...@yahoo.com>> wrote:
> +1 on the general idea to make the reloadability customizable per plugin.
> 
> However, I think it'd be more simple, cleaner and intuitive to not tie it to whether or not a plugin is used both as a global and remap plugin.
> 
> In other words, approach (1) below but, instead of calling it "@global", we could add a param which says "@reloadable=false" (the default value for "@reloadable" can be "true").
> 
> The same param can then be used, when we eventually add relodability to global plugins as well.
> 
> Thoughts?
> 
> 
> 
> 
> On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <solidwallofcode@verizonmedia.com <ma...@verizonmedia.com>> wrote:
> 
> 
> As part of the ATS 9 upgrade, a feature was added so that remap plugins could have their DSO reloaded. This means not just the configuration, but the implementation itself. While very useful, this has some unfortunate side effects with plugins that are used in both a global and remap context. To alleviate this, a configuration variable as added to disable the feature.
> 
> Although reasonable, this is a rather heavy handed way to deal with the problem. What would be better is the ability to reload the DSO or not on a per remap plugin basis. I have a few ways this could be done:
> 
> 1) Add the keyword "@global" to "remap.config". This would behave exactly as "@plugin" except it would prohibit reloading of the DSO for that plugin.
> 
> 2) Have the remap reload configuration check to see if the plugin is also a global plugin and disable remap DSO reload for that plugin.
> 
> 3) Add a flag to the global plugin registry information which can be set during TSPluginInit which disables DSO reloading for that plugin, should it occur in "remap.config". This is similar to (2) but requires a  plugin to prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and would only be valid when called from TSPluginInit.
> 
> 4) As (3), except the flag is set by default and must be cleared to enable DSO reloading in "remap.config".
> 
> I'm willing to see if I can make this work, but I would like to have some feedback on the preferred approach first.
> 

Re: Remap plugin DSO reloading - enabling and disabling

[Alan Carroll <so...@verizonmedia.com>]

Leif;

If the plugin can be global or remap but not both, I don't see why (2)
limits anything. The entire issue is irrelevant for such plugins, because
the situation of reloading the remap DSO but not the global cannot occur,
In fact, option (3) or (4) would enable detecting this and issuing a
warning.

Approach (1) was my first thought, but I think the problem there is whether
the plugin can work as a global and a reloadable remap is a property of the
plugin implementation, not any particular remap rule. That is, for a
specific plugin, there's really no choice about whether to use "@plugin" or
"@global" - the configuration must get it right or the plugin crashes.
Every time. Every rule. It is for this reason I disagree with Leif's view
the user should decide. The user's opinion is irrelevant - the plugin works
in this mode, or it doesn't. And as our friends at LinkedIn discovered,
some rather basic C++ decisions (such as using static variables) will
prevent a plugin from working in this mode. On the other hand, if the
plugin uses the "User Args" feature then it can work, in which case what's
the point of disabling the DSO reload? Unless the plugin implementor is
concerned about code skew between the global and remap versions, which
again the user is not qualified to decide.

My personal preference is (3), but I suspect after mysterious crashes with
plugins, we will have been happier with (4).

On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda <su...@yahoo.com>
wrote:

> +1 on the general idea to make the reloadability customizable per plugin.
>
> However, I think it'd be more simple, cleaner and intuitive to not tie it
> to whether or not a plugin is used both as a global and remap plugin.
>
> In other words, approach (1) below but, instead of calling it "@global",
> we could add a param which says "@reloadable=false" (the default value for
> "@reloadable" can be "true").
>
> The same param can then be used, when we eventually add relodability to
> global plugins as well.
>
> Thoughts?
>
>
>
>
> On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <
> solidwallofcode@verizonmedia.com> wrote:
>
>
> As part of the ATS 9 upgrade, a feature was added so that remap plugins
> could have their DSO reloaded. This means not just the configuration, but
> the implementation itself. While very useful, this has some
> unfortunate side effects with plugins that are used in both a global and
> remap context. To alleviate this, a configuration variable as added to
> disable the feature.
>
> Although reasonable, this is a rather heavy handed way to deal with the
> problem. What would be better is the ability to reload the DSO or not on a
> per remap plugin basis. I have a few ways this could be done:
>
> 1) Add the keyword "@global" to "remap.config". This would behave exactly
> as "@plugin" except it would prohibit reloading of the DSO for that plugin.
>
> 2) Have the remap reload configuration check to see if the plugin is also
> a global plugin and disable remap DSO reload for that plugin.
>
> 3) Add a flag to the global plugin registry information which can be set
> during TSPluginInit which disables DSO reloading for that plugin, should it
> occur in "remap.config". This is similar to (2) but requires a  plugin to
> prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and
> would only be valid when called from TSPluginInit.
>
> 4) As (3), except the flag is set by default and must be cleared to enable
> DSO reloading in "remap.config".
>
> I'm willing to see if I can make this work, but I would like to have some
> feedback on the preferred approach first.
>
>

Re: Remap plugin DSO reloading - enabling and disabling

[Sudheer Vinukonda <su...@yahoo.com>]

 +1 on the general idea to make the reloadability customizable per plugin.
However, I think it'd be more simple, cleaner and intuitive to not tie it to whether or not a plugin is used both as a global and remap plugin.
In other words, approach (1) below but, instead of calling it "@global", we could add a param which says "@reloadable=false" (the default value for "@reloadable" can be "true").
The same param can then be used, when we eventually add relodability to global plugins as well.
Thoughts?



    On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <so...@verizonmedia.com> wrote:  
 
 As part of the ATS 9 upgrade, a feature was added so that remap plugins could have their DSO reloaded. This means not just the configuration, but the implementation itself. While very useful, this has some unfortunate side effects with plugins that are used in both a global and remap context. To alleviate this, a configuration variable as added to disable the feature.
Although reasonable, this is a rather heavy handed way to deal with the problem. What would be better is the ability to reload the DSO or not on a per remap plugin basis. I have a few ways this could be done:
1) Add the keyword "@global" to "remap.config". This would behave exactly as "@plugin" except it would prohibit reloading of the DSO for that plugin.
2) Have the remap reload configuration check to see if the plugin is also a global plugin and disable remap DSO reload for that plugin.
3) Add a flag to the global plugin registry information which can be set during TSPluginInit which disables DSO reloading for that plugin, should it occur in "remap.config". This is similar to (2) but requires a  plugin to prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and would only be valid when called from TSPluginInit.
4) As (3), except the flag is set by default and must be cleared to enable DSO reloading in "remap.config".
I'm willing to see if I can make this work, but I would like to have some feedback on the preferred approach first.
  

Re: Remap plugin DSO reloading - enabling and disabling

[Leif Hedstrom <zw...@apache.org>]

> On May 7, 2020, at 18:24, Alan Carroll <so...@verizonmedia.com> wrote:
> 
> 
> As part of the ATS 9 upgrade, a feature was added so that remap plugins could have their DSO reloaded. This means not just the configuration, but the implementation itself. While very useful, this has some unfortunate side effects with plugins that are used in both a global and remap context. To alleviate this, a configuration variable as added to disable the feature.
> 
> Although reasonable, this is a rather heavy handed way to deal with the problem. What would be better is the ability to reload the DSO or not on a per remap plugin basis. I have a few ways this could be done:
> 
> 1) Add the keyword "@global" to "remap.config". This would behave exactly as "@plugin" except it would prohibit reloading of the DSO for that plugin.

This was one of the suggestions brought up. I don’t know why it was shutdown.
> 
> 2) Have the remap reload configuration check to see if the plugin is also a global plugin and disable remap DSO reload for that plugin.

I think this is worse, many plugins won’t have problems with a global not being reloaded. Worse, many plugins works as either global or remap,  but not both, and this limits what the user can do.

> 
> 3) Add a flag to the global plugin registry information which can be set during TSPluginInit which disables DSO reloading for that plugin, should it occur in "remap.config". This is similar to (2) but requires a  plugin to prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and would only be valid when called from TSPluginInit.
> 
> 4) As (3), except the flag is set by default and must be cleared to enable DSO reloading in "remap.config".

Either is fine. I think option 1 is by far the easiest and most flexible (let the user decide, not the plugin).

— Leif 
> 
> I'm willing to see if I can make this work, but I would like to have some feedback on the preferred approach first.
>