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

Different approach to address initial pending == 0 #902

Merged
merged 3 commits into from Feb 9, 2022
Merged

Conversation

kozlovic
Copy link
Member

@kozlovic kozlovic commented Feb 9, 2022

This is undoing changes done in PR #901
and makes the changes dicussed in the comments of that PR.

Signed-off-by: Ivan Kozlovic ivan@synadia.com

This is undoing changes done in PR #901
and makes the changes dicussed in the comments of that PR.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic
Copy link
Member Author

kozlovic commented Feb 9, 2022

@ripienaar @derekcollison This is another approach, which I sort of prefer. The addconsumer then js.Subscribe() with special option looked a bit of a hack and was not too comfortable with that.

@derekcollison Before you complain about use of lock in the watcher :-)

  • The history limit is currently hard-coded at 64, so that update callback is not going to fire a lot
  • The contention will be gone as soon as Watch() returns, after that, getting the lock in update() callback - since there is no contention - would be less of an issue.

@coveralls
Copy link

coveralls commented Feb 9, 2022

Coverage Status

Coverage decreased (-0.04%) to 85.257% when pulling 7d7a4fe on kv_updates_v2 into 161162c on main.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

Should be done really only when the library successfully created
the consumer.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@kozlovic kozlovic merged commit 52371e4 into main Feb 9, 2022
@kozlovic kozlovic deleted the kv_updates_v2 branch February 9, 2022 23:15
@@ -1573,6 +1566,7 @@ func (js *js) subscribe(subj, queue string, cb MsgHandler, ch chan *Msg, isSync,
// Since the library created the JS consumer, it will delete it on Unsubscribe()/Drain()
sub.mu.Lock()
sub.jsi.dc = true
sub.jsi.pending = info.NumPending + info.Delivered.Consumer
Copy link
Contributor

Choose a reason for hiding this comment

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

good, and this PR is much nicer thanks.

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.

None yet

4 participants