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

Implement a dynamic reload mechanism for Besu plugins. #261

Conversation

AbdelStark
Copy link
Contributor

@AbdelStark AbdelStark commented Dec 17, 2019

PR description

Changes

  • Added reloadConfiguration method in plugin-api.
  • Added admin_reloadPlugin RPC endpoint.
  • if the first parameter is specified the API will attempt to reload the individual plugin configuration if found in the map.
  • if no parameter is specified the API will attempt to reload all plugins configurations.
  • Added method in BesuPluginContextImpl to retrieve a map of named plugins.

TODO

  • add unit tests to cover new methods and test the new RPC API endpoint.

Signed-off-by: Abdelhamid Bakhta abdelhamid.bakhta@consensys.net

Fixed Issue(s)

fixes BESU-162

EdJoJob and others added 30 commits September 24, 2019 09:02
Signed-off-by: Edward Evans <edward.joshua.evans@gmail.com>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
@rain-on
Copy link
Contributor

rain-on commented Dec 18, 2019

Is there complexity around reloading the encrypted-storage plugin? I.e. do all threads in besu need to stop before the plugin is reloaded?

@ajsutton
Copy link
Contributor

I'm somewhat confused about this - we don't, as far as I know, have any built-in way to change a plugin's configuration at runtime (the only config options we provide are CLI args), so why do we need to have plugins reload their config when it won't have changed? If plugins have to provide their own way to update their config, why wouldn't they just automatically reload when it changes?

I'm guessing I'm just missing some context but it's probably worth adding that info to the PR description for the historical record (particularly for when someone tries to document this).

@AbdelStark
Copy link
Contributor Author

Is there complexity around reloading the encrypted-storage plugin? I.e. do all threads in besu need to stop before the plugin is reloaded?

In the lifecycle we had in mind you can call multiple times reload method between start and stop. The idea is to do this operation on the fly without stopping the plugin. Also this operation does not make sense for all plugins. For instance i think the encrypted-storage might be out of scope for the reloading.

@AbdelStark
Copy link
Contributor Author

AbdelStark commented Dec 19, 2019

I'm somewhat confused about this - we don't, as far as I know, have any built-in way to change a plugin's configuration at runtime (the only config options we provide are CLI args), so why do we need to have plugins reload their config when it won't have changed? If plugins have to provide their own way to update their config, why wouldn't they just automatically reload when it changes?

I'm guessing I'm just missing some context but it's probably worth adding that info to the PR description for the historical record (particularly for when someone tries to document this).

Agreed, the description does not provide enough context. So we have some users using Eventeum and they are not happy with it because they have to add a MongoDB instance into their infrastructure. It is an additional component to maintain and to administrate. Consequently we are extending our event stream plugin to add an underlying persistent DB using the KeyValueStorageFactory provided by Besu services exposed to plugins. One feature they are using is the possibility to define the schema of smart contract events using configuration files that describe the fields of the smart contract event(indexed or not, type of the field,...). The location of this schema-mapping file is passed through CLI but then we load the content in memory and we do the parsing only once for performance reasons. We don't necessarily want to poll the file to check if the content has changed because it is only done through a manual operation from the users. The user currently has to stop Besu nodes in order to re-apply the new schema when they want to add/update smart contract events. They want to be able to do this operation on the fly through a manual process, hence the new RPC endpoint. I hope it makes the need of this PR more clear. Happy to discuss about it more if you still have concerns with it.

…i-dynamic-reload-config

Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>

# Conflicts:
#	ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/AdminJsonRpcMethods.java
@ajsutton
Copy link
Contributor

Thanks for the context. I can see how a reload trigger would be useful there. To me it does sound like it should just be a feature of the specific plugin, not a change to the plugin lifecycle. Mostly I just think it would be a better user experience to have a reload API specific to the plugin - especially as in the future plugins might have multiple things they could reload or other reload options etc.

I'm not strongly opinionated on that and don't have all the background. I can see that multiple plugins might want to reload and this approach would work for that, so I'm happy to go along with whatever you decide.

@AbdelStark
Copy link
Contributor Author

Thanks for the context. I can see how a reload trigger would be useful there. To me it does sound like it should just be a feature of the specific plugin, not a change to the plugin lifecycle. Mostly I just think it would be a better user experience to have a reload API specific to the plugin - especially as in the future plugins might have multiple things they could reload or other reload options etc.

I'm not strongly opinionated on that and don't have all the background. I can see that multiple plugins might want to reload and this approach would work for that, so I'm happy to go along with whatever you decide.

I think i will wait after until after holidays so we have the opportunity to discuss more about the next steps.

AbdelStark and others added 2 commits December 20, 2019 09:42
@AbdelStark AbdelStark marked this pull request as ready for review January 6, 2020 16:40
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
…i-dynamic-reload-config

Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>

# Conflicts:
#	ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/JsonRpcTestMethodsFactory.java
@AbdelStark AbdelStark merged commit 94b26dd into hyperledger:master Jan 7, 2020
@AbdelStark AbdelStark deleted the feature/plugin-api-dynamic-reload-config branch January 7, 2020 16:02
edwardmack pushed a commit to ChainSafe/besu that referenced this pull request Feb 4, 2020
* Implement a dynamic reload mechanism for Besu plugins.

- Added `reloadConfiguration` method in `plugin-api`.
- Added `admin_reloadPlugin` RPC endpoint.
    - if the first parameter is specified the API will attempt to reload the individual plugin if found in the map.
    - if no parameter is specified the API will attempt to reload all plugins.
- Added method in `BesuPluginContextImpl` to retrieve a map of named plugins.


Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
edwardmack pushed a commit to ChainSafe/besu that referenced this pull request Feb 4, 2020
* Implement a dynamic reload mechanism for Besu plugins.

- Added `reloadConfiguration` method in `plugin-api`.
- Added `admin_reloadPlugin` RPC endpoint.
    - if the first parameter is specified the API will attempt to reload the individual plugin if found in the map.
    - if no parameter is specified the API will attempt to reload all plugins.
- Added method in `BesuPluginContextImpl` to retrieve a map of named plugins.


Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: edwardmack <ed@edwardmack.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants