-
Notifications
You must be signed in to change notification settings - Fork 765
Change DefaultSubPendingMsgsLimit comment to reflect actual value #998
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
nats.go
Outdated
@@ -4446,7 +4446,7 @@ func (s *Subscription) ClearMaxPending() error { | |||
|
|||
// Pending Limits | |||
const ( | |||
// DefaultSubPendingMsgsLimit will be 512k msgs. | |||
// DefaultSubPendingMsgsLimit will be 524,288 msgs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not 512Kb?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I assumed using Kb as unit in a non-storage context was not common. Happy to change if you think otherwise.. but when I see a b
I immediately associate storage rather than a discrete number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we change it to 500_000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added another commit.. like so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is just for consistency.. I don't seeing it help with understanding per se, but it could help in the edge case situation where the pending number is being reported and its greater than 512k which could be confusing.