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.

Sign up for GitHub

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

Jump to bottom

js: Improvements for push consumers with flow control #711

Merged
merged 1 commit into from May 4, 2021
Merged

js: Improvements for push consumers with flow control #711

merged 1 commit into from May 4, 2021

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Apr 14, 2021

  • Move responding to flow control messages to be done after latest Ack

  • Set larger pending limits for async subscribers with flow control

Signed-off-by: Waldemar Quevedo wally@synadia.com

derekcollison
derekcollison reviewed Apr 14, 2021
View reviewed changes
js.go Outdated Show resolved Hide resolved
js.go Outdated Show resolved Hide resolved
@wallyqs wallyqs force-pushed the js-flow-improv branch 5 times, most recently from 5f8dbca to d1342e5 Compare April 14, 2021 06:12
@wallyqs wallyqs marked this pull request as ready for review April 14, 2021 06:12
@wallyqs wallyqs force-pushed the js-flow-improv branch 2 times, most recently from b08c90f to 4bfa152 Compare April 28, 2021 18:46
wallyqs
wallyqs commented Apr 28, 2021
View reviewed changes
js.go Outdated Show resolved Hide resolved
wallyqs
wallyqs commented Apr 28, 2021
View reviewed changes
js.go Outdated Show resolved Hide resolved
@wallyqs wallyqs force-pushed the js-flow-improv branch 3 times, most recently from 33b9a25 to d4a2b14 Compare April 28, 2021 19:21
wallyqs
wallyqs commented Apr 28, 2021
View reviewed changes
test/js_test.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Apr 28, 2021 •
edited

Coverage Status

Coverage decreased (-0.02%) to 86.876% when pulling 6b98b3e on js-flow-improv into aa78bbf on master.

@wallyqs
Copy link
Member Author

wallyqs commented Apr 28, 2021

Rebased to latest and build is green now

derekcollison
derekcollison reviewed Apr 28, 2021
View reviewed changes
js.go Outdated Show resolved Hide resolved
js.go Outdated Show resolved Hide resolved
derekcollison
derekcollison reviewed Apr 28, 2021
View reviewed changes
js.go Outdated Show resolved Hide resolved
@variadico variadico force-pushed the js-flow-improv branch 3 times, most recently from ac8ccdf to 563a036 Compare May 3, 2021 21:00
derekcollison
derekcollison requested changes May 3, 2021
View reviewed changes
js.go Outdated Show resolved Hide resolved
js.go Outdated Show resolved Hide resolved
nats.go Outdated Show resolved Hide resolved
@variadico variadico force-pushed the js-flow-improv branch 3 times, most recently from 4e364a2 to 8884fab Compare May 3, 2021 22:48
@variadico
Copy link
Contributor

Hm. Seeing this error on CI and locally. Not exactly sure why that's happening. Looking into it.

$ go test -count=5 -run TestJetStreamPushFlowControl_SubscribeAsyncAndChannel
--- FAIL: TestJetStreamPushFlowControl_SubscribeAsyncAndChannel (13.98s)
    js_test.go:1513: Expected 33072 messages, got: 14
FAIL
exit status 1
FAIL    github.com/nats-io/nats.go/test 28.207s

@variadico variadico force-pushed the js-flow-improv branch 5 times, most recently from 7a1cbc3 to 7ed53a6 Compare May 3, 2021 23:25
@wallyqs @nsurfer
js: Improvements for push consumers with flow control
6b98b3e 
Signed-off-by: Waldemar Quevedo <wally@synadia.com>
@variadico variadico requested a review from kozlovic May 4, 2021 00:41
@variadico
Copy link
Contributor

Woo, green! Squashed and rebased commits. Thanks for the help, @derekcollison!

@derekcollison derekcollison self-requested a review May 4, 2021 00:51
derekcollison
derekcollison approved these changes May 4, 2021
View reviewed changes
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 6346f38 into master May 4, 2021
@kozlovic kozlovic deleted the js-flow-improv branch May 4, 2021 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@variadico variadico variadico left review comments

@derekcollison derekcollison derekcollison approved these changes

@kozlovic kozlovic kozlovic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@wallyqs @coveralls @variadico @derekcollison @kozlovic