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