rpc: add subscription id to events #6386
Conversation
tychoish
requested review from
alexanderbez,
cmwaters,
ebuchman and
tessr
as code owners
Codecov Report
@@ Coverage Diff @@
## master #6386 +/- ##
==========================================
- Coverage 65.99% 61.21% -4.78%
==========================================
Files 234 295 +61
Lines 20269 27914 +7645
==========================================
+ Hits 13376 17087 +3711
- Misses 5825 9110 +3285
- Partials 1068 1717 +649
|
tessr
changed the title
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
I've just added some comments in passing as I try to understand the changes. I'm assuming this is to solve the case in #3931
The issue I understand is that we want to generate id's so that clients can cancel individual subscriptions. AFAICS, the query itself serves currently as the unique identifier for subscribing and unsubscribing i.e. a single client can subscribe to multiple queries. From this, I assume this PR therefore targets the case when a single client wants multiple subscriptions to the same query. This appears to me a bit illogical - why would I want to have multiple subscriptions of the same query? Even if multiple services where using the same query on the client side can't they handle it? Maybe I've misunderstood something
libs/pubsub/pubsub.go
Outdated
| @@ -97,7 +116,7 @@ type Server struct { | |||
| // check if we have subscription before | |||
| // subscribing or unsubscribing | |||
| mtx tmsync.RWMutex | |||
| subscriptions map[string]map[string]struct{} // subscriber -> query (string) -> empty struct | |||
| subscriptions map[string]map[string]string // subscriber -> query/id (string) -> id/query (string) | |||
There was a problem hiding this comment.
Does this mean it can be subscriptions[subscriber][query] -> id as well as subscriptions[subscriber][id] -> query`. I'm a little confused with the comments
There was a problem hiding this comment.
yeah, subscriptions are tracked both by query and by ID in an effort to avoid breaking compatibility.
| if err != nil { | ||
| return fmt.Errorf("failed to parse query: %w", err) | ||
| args.ID = query |
There was a problem hiding this comment.
Is this / will this always be the correct way to handle this error?
There was a problem hiding this comment.
I'm adding a comment.
I think it's fine because it'll eventually be an error if it's a bogus ID and a bogus query, but eventually when we remove subscription we can change this (in some way?)
Co-authored-by: Callum Waters <cmwaters19@gmail.com>
tychoish
force-pushed
the
rpc-subscription-id
branch
from
ff4b2cb
to
d863dde
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
|
|
is this ready to be merged? |
Addresses #3931