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

[CHANGED] JetStream: flow control requires heartbeats #2533

Merged
merged 1 commit into from Sep 16, 2021
Merged

Conversation

kozlovic
Copy link
Member

The server will now reject the creation of a push consumer with
flow control if no heartbeat is set.

Resolves #2520

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

The server will now reject the creation of a push consumer with
flow control if no heartbeat is set.

Resolves #2520

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 - Did you check with R.I. on introducing the new error? Apparently there is a process.

@kozlovic
Copy link
Member Author

Did you check with R.I. on introducing the new error? Apparently there is a process.

Yes, which I followed: nats errors add server/errors.json from the root repo. Then add my constant name, code and the error code is picked for me, etc..
Then go generate and that's it.

Copy link
Contributor

@scottf scottf left a comment

Choose a reason for hiding this comment

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

Not directly related, but I'm just wondering if there is a minimum heartbeat that makes sense. Like a heartbeat every 100 ms seems like overkill, but maybe there is a use case for it. I mean getting a heartbeat every 100ms seems like a lot of unnecessary traffic, Maybe there should be a minimum?

server/errors.json Show resolved Hide resolved
@@ -152,6 +152,9 @@ const (
// JSConsumerWQRequiresExplicitAckErr workqueue stream requires explicit ack
JSConsumerWQRequiresExplicitAckErr ErrorIdentifier = 10098

// JSConsumerWithFlowControlNeedsHeartbeats consumer with flow control also needs heartbeats
JSConsumerWithFlowControlNeedsHeartbeats ErrorIdentifier = 10108
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a scheme to error ids? Ask RI I suppose

@ripienaar
Copy link
Contributor

ripienaar commented Sep 15, 2021

Heartbeats also useful in some leader election scenarios, and, at least in tests, very short intervals help

no real schema for constants just make them obvious

Copy link
Contributor

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison
Copy link
Member

Agree we should have a sane minimum. Default for directs for sources and mirrors is 2s, I think OrderedConsumer is 5s.

I might offer 500ms as lowest, but maybe even 1s.

@kozlovic kozlovic merged commit 47359c4 into main Sep 16, 2021
@kozlovic kozlovic deleted the js_cons_fc_hb branch September 16, 2021 16:20
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.

Jetstream: Require Heartbeat when Flow Control is Set or provide alternative
4 participants