-
Notifications
You must be signed in to change notification settings - Fork 912
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
wait command for generic events, invoices first #6127
wait command for generic events, invoices first #6127
Conversation
62be0ad
to
29962cd
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.
I think that I develop my idea of how the paginator API should look, so I share the idea with you maybe we can converge
In my mind, I would like to have a paginator
macro or something that intercepts the json rpc command to check if inside the request there are parameters like limit
, offset
and then decode it inside a jsonrpc_paginator
that I can access with cmd->paginator
I also think that having this struct around the paginator will help to implement smarter ways to filter, such as returning a list in the reverse order this is useful for UX or filter by fields by mapping json field <-> SQL colum
I had a complete history of my idea implemented here https://github.com/vincenzopalazzo/lightning/commits/macros/rpc_filtering but I am not happy with how the interceptor works!
Any thought?
P.S: my code is trash, just I was coding while thinking
P.S': I also think that having this paginator struct can help the code generation by putting inside the json schema a flag is_paginator
or something and the code gen will deal with it
lightningd/invoice.c
Outdated
@@ -1248,7 +1250,7 @@ static void json_add_invoices(struct json_stream *response, | |||
} | |||
} else { | |||
memset(&it, 0, sizeof(it)); | |||
while (wallet_invoice_iterate(wallet, &it)) { | |||
while (wallet_invoice_iterate(wallet, &it, listindex, liststart)) { |
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.
Mh! we always avoid putting specific fields for specific filtering, and I think this was the right choice?
Why we do not steal your idea of filtering
but inject the field inside the JSON RPC params array?
I wrote an I think modular version of the paginator here vincenzopalazzo@7401722#diff-401ba4b6734345a7b89101b868e5709aa58d0e6d6e33b6df2451bbfb488e087f by stealing your idea of filter
filed.
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.
There are several things here:
- We need to drive the offset and limit all the way into the db code for efficiency.
- We need to have a well-defined stable order (this patch defines two in fact, creation and update order) otherwise pagination can occasionally miss entries.
Now, we could hide these, as we did for filtering, but I don't think that's a good idea, since they only apply to a small subset of commands, and you probably really care if they are ignored!
We are unlikely to implement generic output selection, simply because doing it efficiently means we have to have a powerful db engine which maintains a huge number of indexes for each possible query. Hence I expect all usage in practice will be either a single "key" query, or built by keeping your own index and using wait
to detect changes, additions or deletions.
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.
We are unlikely to implement generic output selection, simply because doing it efficiently means we have to have a powerful db engine which maintains a huge number of indexes for each possible query. Hence I expect all usage in practice will be either a single "key" query, or built by keeping your own index and using wait to detect changes, additions or deletions.
Yeah, I agree I was worried into abuse much of this dynamic filtering, but I think we need some way to decide a runtime if I want the result in reverse order or filtering by timestamp.
I had a use case where I would like to have the forwarding by channel id or by timestamp, and maybe have a way to query commands like listnode
and listpeer
in a batch
way! like listnodes batch=[".....", "...."]
We need to have a well-defined stable order (this patch defines two in fact, creation and update order) otherwise pagination can occasionally miss entries.
I guess that I am missing the case where we miss an entry but maybe I am missing just the use case where this happens.
optional string label = 1; | ||
optional string invstring = 2; | ||
optional bytes payment_hash = 3; | ||
optional string offer_id = 4; | ||
optional ListinvoicesIndex index = 5; |
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.
Small question here:
Would it also be an option to remove the enum and have:
message ListinvoicesRequest {
optional string label = 1;
optional string invstring = 2;
optional bytes payment_hash = 3;
optional string offer_id = 4;
optional uint64 start_created = 5;
optional uint64 start_updated = 6;
optional uint64 limit = 7;
}
Most likely I'm missing something ;-)
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.
These are generated, I have no idea: @cdecker ?
Would it be possible to add this to milestone v23.08: https://github.com/ElementsProject/lightning/milestone/30 ? |
It'll be cool if this can be expanded to hooks as well where we could also respond. |
I've implemented invoices, and I am hoping to get listsendpays for 23.08. listpays itself is harder, since:
I think it might Just Work, but I could be wrong... |
29962cd
to
c3f2480
Compare
I'm not sure how that would work? In particular, hooks freeze progress, and we generally don't want that. But this replaces notifications in some cases. |
5a30100
to
0e3ade2
Compare
This will initially be for listinvoices, but can be expanded to other list commands. It's documented, but it makes promises which currently don't exist: * listinvoice does not support `index` or `start` yet. * It doesn't actually fire when invoices change yet. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Added: JSON-RPC: `wait`: new generic command to wait for events.
We set next_<tablename>_<indexname>_index as separate var fields. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We do expirations inside the loop, so we can set updated_index and trigger the callback. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And use a simple array (it's not huge). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…leted. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now we have defined ordering, we can add a start param. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Added: JSON-RPC: `listinvoices` has `index` and `start` parameters for listing control.
Changelog-Added: JSON-RPC: `listinvoices` has `limit` parameter for listing control. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If you miss a wait event, you can catch up by doing listinvoices and getting the max of these fields. It's also a good debugging clue. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
0e3ade2
to
1dc1837
Compare
Based on #6379Merged, so rebased.The first commit contains the lightning-wait(7) documentation, and the rest of it is simply delivering the promises!
If this API passes muster, we can expand it to other list commands in future releases.