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 <[email protected]> > 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 >> <[email protected]> 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 >>>> <[email protected]> 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 <[email protected]> wrote: >>>> >>>> >>>>> On May 7, 2020, at 8:12 PM, Alan Carroll >>>>> <[email protected]> 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 >>>>>> <[email protected]> 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 >>>>>> <[email protected]> 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. >>>>>> >>>>
