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 <[email protected]> wrote: > > > > >>> On May 9, 2020, at 14:53, Alan Carroll <[email protected]> >>> 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 >>> <[email protected]> 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 <[email protected]> wrote: >>>>> >>>> >>>> >>>>> On May 8, 2020, at 2:29 PM, Alan Carroll >>>>> <[email protected]> wrote: >>>>> >>>>> >>>>> >>>>>> On Fri, May 8, 2020 at 3:17 PM Leif Hedstrom <[email protected]> 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 >>>>
