Skip to content

Conversation

bruth
Copy link
Member

@bruth bruth commented Jun 13, 2022

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.

Copy link
Member

@wallyqs wallyqs left a 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.
Copy link
Member

Choose a reason for hiding this comment

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

Why not 512Kb?

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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?

@coveralls
Copy link

coveralls commented Jun 13, 2022

Coverage Status

Coverage increased (+0.3%) to 84.188% when pulling ed2c6f6 on bruth:patch-2 into dcbb65a on nats-io:main.

Copy link
Collaborator

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

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

LGTM

@piotrpio piotrpio merged commit ea3ef92 into nats-io:main Jul 9, 2025
@piotrpio piotrpio mentioned this pull request Jul 29, 2025
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.

5 participants