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

Add a new CLI option to limit the number of requests in a single RPC batch request #4965

Merged

Conversation

Gabriel-Trintinalia
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia commented Jan 19, 2023

PR description

Adds a --rpc-http-max-batch-size CLI option to restrict the maximum number of requests in a single RPC batch request.

Default 50
When -1: unlimited - the current configuration.

When number of requests in batch > --rpc-http-max-batch-size:

  • Throw a JsonRpcError.EXCEEDS_RPC_MAX_BATCH_SIZE; with type INVALID_PARAM and message Number of requests in batch exceeds --rpc-http-max-batch-size.

Fixed Issue(s)

fixes #4951

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

Gabriel-Trintinalia and others added 3 commits January 19, 2023 13:21
Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
@@ -86,6 +86,8 @@ public interface DefaultCommandValues {
int DEFAULT_P2P_PEER_LOWER_BOUND = 25;
/** The constant DEFAULT_HTTP_MAX_CONNECTIONS. */
int DEFAULT_HTTP_MAX_CONNECTIONS = 80;
/** The constant DEFAULT_HTTP_MAX_BATCH_SIZE. */
int DEFAULT_HTTP_MAX_BATCH_SIZE = 50;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should mirror Geth and other clients at 1 IMO - users that leave this exposed by default will not want such a large batch size and those that need the functionality can track down this CLI option in the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mirroring @macfarla comment's on #4951

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
return JsonRpcExecutorHandler.handler(jsonRpcExecutor, tracer);
final JsonRpcExecutor jsonRpcExecutor,
final Tracer tracer,
final JsonRpcConfiguration configuration) {
Copy link
Contributor

@gfukushima gfukushima Jan 20, 2023

Choose a reason for hiding this comment

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

nit: suggestion of variable name to follow the same pattern -> jsonRpcConfiguration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@gfukushima gfukushima left a comment

Choose a reason for hiding this comment

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

LGTM

@macfarla macfarla merged commit 0626acd into hyperledger:main Jan 22, 2023
@macfarla macfarla added the doc-change-required Indicates an issue or PR that requires doc to be updated label Jan 31, 2023
@Gabriel-Trintinalia Gabriel-Trintinalia deleted the 4951-rpc-http-batch-size-limit branch January 31, 2023 23:56
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Feb 1, 2023
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
…batch request (hyperledger#4965)

* Add option to limit requests in a single batch

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

* Change changelog

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

* Set default max batch size to one

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

* Update changelog

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

* Fix max rpc batch size for unit tests

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

* Change variable name

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
…batch request (hyperledger#4965)

* Add option to limit requests in a single batch

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

* Change changelog

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

* Set default max batch size to one

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

* Update changelog

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

* Fix max rpc batch size for unit tests

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

* Change variable name

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>

Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a new CLI option to limit the number of requests in a single RPC batch request
5 participants