> 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 >>>
