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

msggen: add batching method #7255

Closed
wants to merge 1 commit into from

Conversation

daywalker90
Copy link
Contributor

Nothing special here, adds:

  • batching

@daywalker90 daywalker90 requested a review from cdecker as a code owner April 23, 2024 15:58
@cdecker cdecker enabled auto-merge (rebase) April 24, 2024 09:38
@cdecker
Copy link
Member

cdecker commented Apr 25, 2024

This does not what it says on the tin:

The batching RPC command allows (but does not guarantee!) database
commitments to be deferred when multiple commands are issued on this RPC
connection. This is only useful if many commands are being given at once, in
which case it can offer a performance improvement (the cost being that if
there is a crash, it's unclear how many of the commands will have been
persisted).

Notice that this sets the current connection up to be batching, avoiding DB transactions being opened and closed for each following command. However, the cln-rpc crate uses a new JSON-RPC connection for each command, so this will set the connection to batching, but then immediately discard it again. There is no good concept of connection in grpc clients either, so this is a method that is not applicable outside of direct clients or plugins (for which this was actually created by the way).

We should document this (in a RPC comparison matrix? 🤔 ) and call it out as not applicable.

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

This methods is not applicable except for local clients.

@daywalker90
Copy link
Contributor Author

Interesting, i thought if you used e.g. .call() multiple times on the same ClnRpc variable it would be one and the same rpc connection. So are we not enabling this and instead documenting it somewhere?

@daywalker90
Copy link
Contributor Author

Or just point out the difference between rpc and grpc?

@cdecker
Copy link
Member

cdecker commented Apr 25, 2024

We should likely just not map this method through, and document somewhere the reason for it: it doesn't do what you'd expect.

@cdecker cdecker closed this May 16, 2024
auto-merge was automatically disabled May 16, 2024 12:02

Pull request was closed

@daywalker90 daywalker90 deleted the batching branch May 16, 2024 12:10
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