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

Denylist reload #101

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Denylist reload #101

wants to merge 13 commits into from

Conversation

macfarla
Copy link
Contributor

@macfarla macfarla commented Oct 14, 2024

allow config reload for LineaTransactionPoolValidatorPlugin.

Simplest possible implementation with no optimization.

Built on top of #99

Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
log.info(
"Registering Linea plugin of type {} with name {}",
this.getClass().getName(),
this.getName());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if plugin name == class name this extra logging may not be useful

@@ -124,6 +122,12 @@ public void start() {
}
}

@Override
public CompletableFuture<Void> reloadConfiguration() {
loadDenyListAndRegisterPluginTxValidatorFactory();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to look at rejectedTxJsonRpcManager - may need to do something different with that re start/stop lifecycle

Copy link
Contributor

Choose a reason for hiding this comment

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

factory field in TransactionPoolValidatorServiceImpl is not volatile so the change could not be seen immediately by all thread, said that I prefer that the registration is done one, and the reload only updates the deny list, so for example restricting the moving pieces to a wrap of the deny list in a mutable container that is set at startup and updated on every reload, without having to redo all the initialization of the plugin

@macfarla macfarla marked this pull request as draft October 14, 2024 06:36
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

Nice to learn that we already have a way to trigger a reload of the plugin conf.

Shared a comment with my view on how the conf reload should only touch the deny list

@@ -124,6 +122,12 @@ public void start() {
}
}

@Override
public CompletableFuture<Void> reloadConfiguration() {
loadDenyListAndRegisterPluginTxValidatorFactory();
Copy link
Contributor

Choose a reason for hiding this comment

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

factory field in TransactionPoolValidatorServiceImpl is not volatile so the change could not be seen immediately by all thread, said that I prefer that the registration is done one, and the reload only updates the deny list, so for example restricting the moving pieces to a wrap of the deny list in a mutable container that is set at startup and updated on every reload, without having to redo all the initialization of the plugin

Signed-off-by: Sally MacFarlane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants