-
Notifications
You must be signed in to change notification settings - Fork 29
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
Replace HAResources with TieredLimits #169
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 with the exception that I think we are pulling out a level of the limits, when we already have where to put them in.
v2/account_claims.go
Outdated
// OperatorLimits are used to limit access by an account | ||
type OperatorLimits struct { | ||
NatsLimits | ||
AccountLimits | ||
JetStreamLimits | ||
TieredLimits `json:"tiered_limits,omitempty"` |
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.
Since these are ultimately JetStream limits not sure why don't we group them with JetStreamLimits.
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.
so we don't get recursive tiers.
It's either move tieredlimits into JetStream Limits but then the actual values have to be moved into their own struct.
Or keep them next to each other.
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.
Not sure yet if lgtm...
v2/account_claims.go
Outdated
@@ -72,26 +71,36 @@ func (j *JetStreamLimits) IsJSEnabled() bool { | |||
return j.MemoryStorage != 0 || j.DiskStorage != 0 | |||
} |
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.
The fact that TieredLimits is not part of JetStreamLimits (although they are JS limits) as Alberto suggested, means that JetStreamLimits.ISJSEnabled() would return false here, even if there are tiered limits. Is that correct?
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 will push an update with refactored limits, so we can look at the alternative.
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.
Turns out IsJSEnabled was added with the previous (HAResources commit) and has not been released.
@@ -61,7 +60,7 @@ type JetStreamLimits struct { | |||
|
|||
// IsUnlimited returns true if all limits are unlimited | |||
func (j *JetStreamLimits) IsUnlimited() bool { | |||
return *j == JetStreamLimits{NoLimit, NoLimit, NoLimit, NoLimit, NoLimit, false} | |||
return *j == JetStreamLimits{NoLimit, NoLimit, NoLimit, NoLimit, false} |
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.
JetStreamLimits will be mapped to R factor in the future, right?
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.
Yes, for now we will be indexing with tier names "R1" and "R3".
However, that indexing gives us the flexibility to change that going forward.
You can see a sample of this in the attached unit test
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
TieredLimits are mutually exclusive with JetStream limits Signed-off-by: Matthias Hanel <mh@synadia.com>
Signed-off-by: Matthias Hanel <mh@synadia.com>
Signed-off-by: Matthias Hanel <mh@synadia.com>
Signed-off-by: Matthias Hanel <mh@synadia.com>
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
TieredLimits are mutually exclusive with JetStream limits
Signed-off-by: Matthias Hanel mh@synadia.com