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.

Sign up for GitHub

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

Jump to bottom

plugin API rework, plugin status support #583

Merged
merged 123 commits into from
Jan 23, 2024
Merged

plugin API rework, plugin status support #583

merged 123 commits into from
Jan 23, 2024

Conversation

milyin
Copy link
Contributor

@milyin milyin commented Nov 9, 2023 •
edited
Loading

  • internal change: hash of validator closures replaced to single trait - to simplify RunningPluginTrait
  • plugin compatibility support improved. Now it compares
    • rust version
    • structures used versions
    • build features affecting structures
  • PluginManager rework
    • added ability to keep information of plugins not loaded yet
    • added ability to request plugin status
    • pluign manager API made easier to use. Separate traits for declared, loaded and started plugins allows to avoid repetitive error checks
    • Used same plugin manager in both zenohd and storage magager plugin

Corresponging PRs:
eclipse-zenoh/zenoh-backend-influxdb#54
eclipse-zenoh/zenoh-backend-rocksdb#40
eclipse-zenoh/zenoh-backend-filesystem#46
eclipse-zenoh/zenoh-plugin-webserver#32
eclipse-zenoh/zenoh-plugin-mqtt#32
eclipse-zenoh/zenoh-plugin-dds#168
eclipse-zenoh/zenoh-plugin-ros1#30
eclipse-zenoh/zenoh-plugin-ros2dds#51

@milyin
made runtime state fields private
0164dd8
@milyin
RuntimeState private
e0dbcf5
@milyin
removing ValidationFunction unfinished
47daf4c
@milyin
make validator function
b920073
@milyin
removed validation function
735ae2b
@milyin
removed cyclic reference
c4e95b9
@milyin
format fix
56f43be
@milyin
clippy fix
8d69198
@milyin
Merge branch 'master' into runtime_blob
def94fb
@milyin
unfinished
27ebcd5
@milyin
zenoh-plugin-trait corrected
1dc7662
@milyin
version control unfinished
c059cc0
@milyin
loader compatibility version
ee44f0e
@milyin
string version with features
b1780e1
@milyin
concat_enabled_features
63346f0
@milyin
example storage plugin
a91e0cf
@milyin
config corrected for example storage plugin
5fdbe66
@milyin
example storage plugin works as memory one
be46e36
@milyin
support load api from example stroage plugin
89e67dd
@milyin
unifying plugn loading unfinished
84442f5
@milyin
replaced volumes map to plugin_manager
684c279
@milyin
static memory unfinihed
3f23cbc
@milyin
plugin manager refactor
41f5216
@milyin
plugin manager api update
84498a4
@milyin
running plugin index added
ef7bfaa
@milyin
safer plugin load api, unfinished
7d4856b
@milyin
compiles
6a84494
@milyin
static memory plugin fix
21f03a8
@milyin
compare version improved
fafb5cd
@milyin
moved feature list to crate level
f080df6
@milyin milyin marked this pull request as ready for review January 18, 2024 14:34
@milyin milyin mentioned this pull request Jan 18, 2024
Merged
Mallets
Mallets reviewed Jan 23, 2024
View reviewed changes
plugin_test_config.json5 Outdated Show resolved Hide resolved
Mallets
Mallets reviewed Jan 23, 2024
View reviewed changes
plugins/zenoh-plugin-trait/src/plugin.rs Outdated
pub trait PluginConditionSetter {
fn add_error(self, report: &mut PluginReport) -> Self;
fn add_warning(self, report: &mut PluginReport) -> Self;
fn add_message(self, report: &mut PluginReport) -> Self;
Copy link
Member

Choose a reason for hiding this comment

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

As log levels we have "info, warn, err ,debug, trace". Do add_error, add_warning, and add_message refer to those levels? If so, would add_message better be named as add_info?

Copy link
Contributor Author

@milyin milyin Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, this is related to plugin's error report level:

pub enum PluginReportLevel {
    #[default]
    Normal,
    Warning,
    Error,
}

I'll think about giving names which shows this relation more clear.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, maybe Normal could be renamed to Info then? What do you think?

Copy link
Contributor Author

@milyin milyin Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agree. This enum was PluginStatus before, the Normal remained from this. Now Info is ok here.

Mallets
Mallets reviewed Jan 23, 2024
View reviewed changes
zenoh/src/net/routing/face.rs
@@ -31,6 +31,7 @@ pub struct FaceState {
pub(super) id: usize,
pub(super) zid: ZenohId,
pub(super) whatami: WhatAmI,
#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

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

Why is local a dead_code now? What kind of changes are introduced that make local dead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a side-effect of hiding fileds in RuntimeState, which were public before (https://github.com/eclipse-zenoh/zenoh/pull/583/files#diff-d1c4d04e44c0a6c9db07b8efc60893ed69cdcdb7d8c970192a034f94280df339R51-R61) This caused large leak of private structures into public API. So the unused function FaceState::is_local() was incorrectly public before.
Seems that this field and function can be removed, but better to ask @OlivierHecart first. So for now they just marked as dead.

Mallets
Mallets requested changes Jan 23, 2024
View reviewed changes
Copy link
Member

@Mallets Mallets left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me. I've left few minor comments that should be addressed before merging.

@milyin
Copy link
Contributor Author

milyin commented Jan 23, 2024

Overall it looks good to me. I've left few minor comments that should be addressed before merging.

Done with all the comments.

@Mallets Mallets merged commit 8d7a634 into main Jan 23, 2024
15 checks passed
@milyin milyin mentioned this pull request Jan 25, 2024
Closed
@milyin milyin self-assigned this Jan 25, 2024
@Mallets Mallets added the enhancement Existing things could work better label Jan 25, 2024
@Mallets Mallets mentioned this pull request Jan 25, 2024
Open
@fuzzypixelz fuzzypixelz mentioned this pull request Jan 25, 2024
Closed
@JEnoch JEnoch mentioned this pull request Jan 30, 2024
Open
@milyin milyin deleted the simplified_load_plugin branch April 15, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mallets Mallets Mallets approved these changes

@p-avital p-avital Awaiting requested review from p-avital

@OlivierHecart OlivierHecart Awaiting requested review from OlivierHecart

Assignees

@milyin milyin

Labels
enhancement Existing things could work better
Projects
Zenoh 1.0.0 release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@milyin @fuzzypixelz @Mallets