reconciler: Shrink Status type #101
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
eb85019 to
86ffd17
Compare
|
On And in this PR: So for 1M objects with |
derailed
approved these changes
Jul 31, 2025
reconciler/types.go
Outdated
| ID: nextID(), | ||
| } | ||
| } | ||
|
|
||
| // statusError constructs the status that marks the object | ||
| // as failed to be reconciled. | ||
| func StatusError(err error) Status { | ||
| errStr := err.Error() |
There was a problem hiding this comment.
Ah indeed! Fixed and added test.
| StatusKindRefreshing StatusKind = "Refreshing" | ||
| StatusKindDone StatusKind = "Done" | ||
| StatusKindError StatusKind = "Error" | ||
| var ( |
There was a problem hiding this comment.
Could be out of context here but this feels a little odd aka consts -> global variables. Not sure if applicable but perhaps we could have gone with StatusKind uint8 and generate the string equivalent of the consts via go:generate.
Aka have the compiler generate the map vs manually curating statusKindToString
There was a problem hiding this comment.
I wish I could do that, but since we have existing code doing string(foo.Status.Kind) we cannot do type StatusKind uint8 since string(uint8(0)) compiles and we want to force the code to switch to foo.Status.Kind.String(). Maybe in the future once all use-sites have converted over? For now I'd like to err on the safe side and do this trick here to force the use-sites to be fixed.
Also this has to be var instead of const since struct{...} cannot be const.
Error is rarely set, so we can save 8 bytes by using *string instead of string. This brings 'Status' size down to 56 from 64 bytes. Signed-off-by: Jussi Maki <jussi.maki@isovalent.com>
This brings 'Status' size to 48 bytes from 56. The struct fields were rearranged by "betteralign". Signed-off-by: Jussi Maki <jussi.maki@isovalent.com>
86ffd17 to
e4cada2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Errorfromstringto*stringas it's rarely set (64 => 56 bytes)StatusKindfromstringtouint8(56 => 48 bytes)The
StatusKindisstruct { uint8 }to make sure code that used to dostring(foo.Status.Kind)won't compile and is forced to change tofoo.Status.Kind.String(). Note that we're going to release v0.5.0 with few other breaking API changes in addition to this one and tooling in cilium/cilium only bumps StateDB on patch version changes.