Plugin toggle roadmap #132876
Draft
Plugin toggle roadmap #132876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit modifies the PluginInstances class to remove direct access to the m_instances vector. Instead, we expose a new `GetSnapshot` method that returns a copy of the current state of the instances vector. All external iteration over the instances is updated to use the new method. The motivation for the change is to allow modifying the way we store instances without having to change all the clients. This is a preliminary change to allow enabling/disabling of plugins in which case we want to iterate over only enabled plugins. We also considered using a custom iterator that wraps the vector iterator and can skip over disabled instances. That works, but the iterator code is a bit messy with all template and typedefs to make a compliant iterator.
This commit adds support for enabling and disabling plugins by name.
The changes are made generically in the `PluginInstances` class, but
currently we only expose the ability to SystemRuntime plugins. Other
plugins types can be added easily.
We had a few design goals for how disabled plugins should work
1. Plugins that are disabled should still be visible to the system.
This allows us to dynamically enable and disable plugins
and report their state to the user.
2. Plugin order should be stable across disable and enable changes.
We want avoid changing the order of plugin lookup. When a plugin
is re-enabled it should return to its original slot in the
creation order.
3. Disabled plugins should not appear in PluginManager operations.
Clients should be able to assume that only enabled plugins will
be returned from the PluginManager.
We explored two other ways to implement this change:
1. Keep two separate lists and move the instances between them when
enabling and disabling plugins. This requires maintaining the
original insert order to satisfy the goals above and thus adds
some complexity to the code.
2. Return a copy of the instances list with disabled plugins removed
when clients iterate over them. This requires extra copies and
would potentially lead to the copies getting out of sync with
the actual stored instances because any modifications to the
instances would be lost.
Instead, we modify the plugin instance to maintain a bool of its enabled
state. Clients external to Instances class expect to iterate over only
enabled instance so we skip over disabed instances in the query and
snapshot apis. This way the client does not have to manually check which
instances are enabled.
This commit adds three new commands for managing plugins. The `list`
command will show which plugins are currently registered and their
enabled state. The `enable` and `disable` commands can be used to
enable or disable plugins.
A disabled plugin will not show up to the PluginManager when it iterates
over available plugins of a particular type.
The purpose of these commands is to provide more visibility into registered
plugins and allow users to disable plugins for experimental perf reasons.
There are a few limitations to the current implementation
1. Only SystemRuntime plugins are currently supported. We can easily
extend the existing implementation to support more types.
2. Only "statically" know plugin types are supported (i.e. those
managed by the PluginManager and not from `plugin load`). It is
possibly we could support dynamic plugins as well, but I have
not looked into it yet.
dmpots
force-pushed
the
plugin-toggle-roadmap
branch
from
9e71881 to
aaa3ab3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This set of commits shows how we can implement plugin enable/disable. We will create PRs for the individual commits. The purpose of this draft PR is to provide the full context of how it all fits together.