Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(1059): adding explicit plugin id in PluginStatus and PluginManager #1060

Merged
merged 13 commits into from
Jun 3, 2024

Conversation

gabrik
Copy link
Contributor

@gabrik gabrik commented May 29, 2024

This PR adds the id() method to the PluginStatus trait and allows passing it as a parameter in the PluginManager when dynamic loading or statically linking a plugin.
It exposes plugins in the admin space via their ID (see below what the ID is), and allows multiple instances of the same plugin with different configurations, essential for backends, and useful for other plugins.

Changes to configuration:

        "rest2": { // plugin id, used in admin space, to retrieve plugin config
            "__required__": true,
            "http_port": 8000,
            "__plugin__":"rest", // plugin name used in libloading
        },

Sister PRs:

Sister PRs for testing, do not need a merge.

Closes #1059

@gabrik gabrik requested review from JEnoch and milyin May 29, 2024 07:06
@gabrik gabrik added bug Something isn't working breaking-change Indicates that the issue implies a breaking change (be it at compile time or at runtime) labels May 29, 2024
@gabrik
Copy link
Contributor Author

gabrik commented May 29, 2024

FYI: I see too many unwraps in the PluginManager implementation, indeed, they should never fail as they are calls to Vec::last after a Vec::insert but I think they should not be there anyway.

@gabrik
Copy link
Contributor Author

gabrik commented May 29, 2024

Note: Lint is failing on a file that has not been touched by the PR.

@gabrik gabrik requested a review from OlivierHecart May 29, 2024 13:25
@gabrik gabrik marked this pull request as ready for review May 30, 2024 16:42
@Mallets
Copy link
Member

Mallets commented May 31, 2024

The PR looks good to me.
@gabrik should we wait merging it till all sister PRs are made?

@gabrik
Copy link
Contributor Author

gabrik commented May 31, 2024

The PR looks good to me. @gabrik should we wait merging it till all sister PRs are made?

I'm opening the sister PRs and tagging people for review, the ones not to be merged are going to be closed.

@gabrik
Copy link
Contributor Author

gabrik commented May 31, 2024

Once the sisters are approved then I'm going to merge everything.

Copy link
Contributor

@milyin milyin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work ok, but later some refactoring is needed I think, as the name became mostly unnecessary.
The parameters of methods should be renamed (name -> id)

plugins/zenoh-plugin-trait/src/manager.rs Outdated Show resolved Hide resolved
plugins/zenoh-plugin-trait/src/manager.rs Outdated Show resolved Hide resolved
plugins/zenoh-plugin-trait/src/manager.rs Outdated Show resolved Hide resolved
@@ -1021,6 +1021,7 @@ fn load_external_plugin_config(title: &str, value: &mut Value) -> ZResult<()> {
#[derive(Debug, Clone)]
pub struct PluginLoad {
pub name: String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, name and pathshave the same purpose: it's a information for loader where to look for corresponding plugin DLL. Probably the name should near the paths and be optional also

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way it is done now it is already Optional in the configuration as the user needs to specify __plugin__ to specify the name, if not specified the id is used also as name.
Thus current configurations of zenoh will continue to work.

plugins/zenoh-plugin-trait/src/manager.rs Outdated Show resolved Hide resolved
@@ -60,6 +62,11 @@ impl<StartArgs: PluginStartArgs, Instance: PluginInstance> PluginStatus
fn name(&self) -> &str {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that after this update we need this name getter at all. If plugin is dynamic, the path() getter gives us full information where this plugin came from. If it's static, name and id is the same thing.

Copy link
Contributor Author

@gabrik gabrik Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The former is true, while not the latter, the user can statically link the plugin once, but instantiate it multiple times with different IDs, thus the name will be the same but the IDs will be different

Signed-off-by: Gabriele Baldoni <[email protected]>
@gabrik gabrik merged commit c9ef80f into main Jun 3, 2024
16 of 21 checks passed
@gabrik gabrik deleted the fix/1059-add-plugin-id branch June 3, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates that the issue implies a breaking change (be it at compile time or at runtime) bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Spawning more volumes with the same backend is not reflected in admin space
3 participants