-
Notifications
You must be signed in to change notification settings - Fork 253
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
refactor: introduces NamespaceVersionMaxValue const #1893
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1893 +/- ##
=======================================
Coverage 21.85% 21.85%
=======================================
Files 117 117
Lines 13304 13304
=======================================
Hits 2907 2907
Misses 10107 10107
Partials 290 290
|
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.
PR LGTM after a small change
pkg/appconsts/global_consts.go
Outdated
@@ -11,7 +13,8 @@ import ( | |||
// They can not change throughout the lifetime of a network. | |||
const ( | |||
// NamespaveVersionSize is the size of a namespace version in bytes. | |||
NamespaceVersionSize = 1 | |||
NamespaceVersionSize = 1 | |||
NamespaceVersionMaxValue = math.MaxUint8 // this value should be set according to NamespaceVersionSize, i.e., 2^NamespaceVersionSize - 1 |
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 think there's a slight typo in 2^NamespaceVersionSize - 1
b/c it doesn't evaluate to math.MaxUint8
. When NamespaceVersionSize
is interpolated, this evaluates to 2 ^ 1 - 1
. Maybe this meant to say something like ... i.e., // math.Pow(2, 8 * NamespaceVersionSize) - 1
but proposal to do this instead:
// NamespaceVersionMaxValue is the maximum value a namespace version can be.
// This const must be updated if NamespaceVersionSize is changed.
NamespaceVersionMaxValue = math.MaxUint8
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.
You are right, when writing that formula, I was thinking of NamespaceVersionSize
in bits, instead of bytes. The correction is as you suggested 2^(8* NamespaceVersionSize)-1)
. Going to fix it.
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.
Done in 56a8fd2.
[nit] you may change the conventional commit prefix of this PR to |
Overview
Although we already have a constant
NamespaceVersionSize
, the comparison of the max value of the namespace version of the incoming Blobs is currently hardcoded tomath.uint8
. While this aligns with the current value ofNamespaceVersionSize
which is 1 byte, it would be safer (and also easier to debug and maintain the code) to derive the maximum value from the original constant instead of manually hardcoding it. This PR incorporates this modification.cc: @rootulp @evan-forbes please let me know if this change also makes sense to you.
Checklist