gtenev commented on a change in pull request #6880:
URL: https://github.com/apache/trafficserver/pull/6880#discussion_r446284983
##########
File path: proxy/http/remap/PluginFactory.cc
##########
@@ -82,7 +82,7 @@ RemapPluginInst::osResponse(TSHttpTxn rh, int
os_response_type)
_plugin.osResponse(_instance, rh, os_response_type);
}
-PluginFactory::PluginFactory()
+PluginFactory::PluginFactory(const DLL<PluginRegInfo> &globalPluginRegistry)
Review comment:
I wonder if we can redesign this to be simpler and more modular and
avoid the unnecessary (AFAICT) coupling between the global plugin loading and
remap plugin loading subsystems (please my next comment).
##########
File path: proxy/http/remap/PluginFactory.cc
##########
@@ -94,6 +94,12 @@ PluginFactory::PluginFactory()
}
}
+ for (PluginRegInfo *plugin_info = globalPluginRegistry.head; plugin_info !=
nullptr; plugin_info = (plugin_info->link).next) {
Review comment:
For this to work:
* `PluginRegInfo` had to be enhanced with `dso_reloading_enabled` flag
* `plugin_reg_list` had to be passed to `PluginFactory` constructor
* `PluginFactory` had to understand global `PluginRegInfo` internals.
* `PluginFactory` would need to rebuild the local registry `optoutPlugins`
on each config reload (by traversing `plugin_reg_list`)
May be we can simplify this design (avoid all of the above) and get rid of
the unnecessary coupling with the following idea?
We could move the local `optoutPlugins` registry from `PluginFactory` into
`LoadedPlugins`, a structure that already exists and survives the
`PluginFactory` instances, which is what we actually need.
We could add a method or methods to `LoadedPlugins` interface to manipulate
the registry from within `TSPluginDSOReloadEnable` and then expose another
`LoadedPlugins` method to be called from the `PluginFactory` to check if the
plugin opted out from reloading when the remap plugin is instantiated.
@brbzull0, please feel free to ping me out-of-band, I will be glad to
discuss!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
