improve: refining resource version comparison #3016
Conversation
Signed-off-by: Steve Hawkins <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR refines the resource version comparison logic to improve both performance and correctness. The implementation is simplified to compare resource versions as numeric strings more efficiently, achieving approximately 45% performance improvement over the previous approach while maintaining proper validation.
Key Changes:
- Extracted validation logic into a separate helper method for better code organization
- Simplified comparison algorithm by first comparing lengths, then character-by-character when lengths match
- Consolidated error messages to remove redundant "(1)" and "(2) suffixes
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (char1 == '0') { | ||
| if (i == 0) { | ||
| throw new NonComparableResourceVersionException( | ||
| "Non numeric characters in resource version (2): " + char2); | ||
| } | ||
| if (char1 == 0) { | ||
| comparison = -1; | ||
| } else if (comparison == 0) { | ||
| comparison = Character.compare(char1, char2); | ||
| "Resource version cannot begin with 0: " + v1); | ||
| } |
There was a problem hiding this comment.
The validation logic incorrectly allows '0' characters at any position except the first. This means resource versions like '10', '20', '100' would be valid, but '101' would fail validation when it encounters the '0' at position 1. The condition should only validate the leading zero case, not reject all '0' characters.
csviri
changed the title
|
Sorry I did not mean to merge this yet, but since cannot easily undo, will stick with it |
2 participants
After expanding the micro benchmark to include warm-up, and a variety of comparison scenarios, it does appear there are a couple of improvements that can be made. This seems to be the optimal and most straight-forward.
It is about 45% faster than using parseLong, but that still may not be good enough to rely upon in github actions without warmup due to garbage collection and or cpu throttling.
Another reason, besides performance, to not use parseLong is that I believe the Kubernetes folks are allowing for the possiblity of arbitrary length resourceVersions.
cc @metacosm @csviri