[ADDED] StatusChanged for core and js subscriptions #1570
Conversation
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio
force-pushed
the
blocking-drain
branch
from
cab4f81
to
075667d
Compare
nats.go
Outdated
| return drainCh | ||
| } | ||
|
|
||
| func (s *drainStatus) PendingMsgs() int { |
There was a problem hiding this comment.
can get this from sub.Pending() already for both PendingMsgs abd PendingBytes, maybe not needed?
nats.go
Outdated
| } | ||
| } | ||
|
|
||
| func (s *drainStatus) Draining() bool { |
There was a problem hiding this comment.
could be part of subscription state instead sub.IsDraining maybe? like the sub companion for nc.IsDraining
There was a problem hiding this comment.
Yes, I suppose getting rid of DrainStatus() method and hanging everything off from Subscription is a good call. At first I had a different idea around structuring it and left the API it as it was. I'll change that.
nats.go
Outdated
| defer s.sub.mu.Unlock() | ||
| if s.sub.drainingComplete == nil { | ||
| drainCh = make(chan struct{}) | ||
| s.sub.drainingComplete = drainCh |
There was a problem hiding this comment.
drainingComplete maybe too long, could be instead sub.drainCh like here or sub.dch
There was a problem hiding this comment.
drainCh sounds good, I'll change it. dch is a bit too ambiguous for my liking.
test/drain_test.go
Outdated
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
| sub.Drain() | ||
| ds := sub.DrainStatus() |
There was a problem hiding this comment.
maybe later can have some helper build on top like sub.WaitDrained(ctx) error that waits for drain to complete or timeout.
There was a problem hiding this comment.
Yeah, that's a good idea. Not sure I want to add it in this PR though, it's easy enough to implement it yourself and I want to be careful about adding public APIs.
In the future though, that might be useful.
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
|
I changed the approach a bit and instead of a method for listening for draining completion on subscription I've added a This allows getting notifications on several events on a subscription - sub active, draining, closed and slow consumer for now. This matches what we have for conn and seems to be a more robust solution than only checking drain status. @Jarema @wallyqs can you take a look and give me your thoughts? |
piotrpio
changed the title
|
I really like this approach, as it looks way more consistent with how we handle those events. |
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski piotr@synadia.com