-
Notifications
You must be signed in to change notification settings - Fork 13
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
adding batching options for RPC client #72
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
9af06f4
to
56dfebb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @Chengxuan - please see the feedback on the config/API which I think is mandatory to apply.
The algorithm and config spelling, please have a think about.
I do think the batch timer starting from the first message, rather than being a tick, is worth considering
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
@peterbroadhurst this PR is ready to be reviewed again. Thanks for the detailed review. I did start my PR based on the batching logic in txwritter. But then I realized the logic here doesn't need to get as complex as such, a queue-based dynamic dispatch thread allocation with a concurrency control is sufficient. it avoids unnecessary dangling go routine and simplifies the logic for the reader to read in my view. |
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
This PR adds an option to leverage JSON-RPC batching feature.
The batch option is disabled by default and the following parameters can be configured to tune its behaviour: