Resource version comparison utility #2988
Conversation
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
|
Should this include the numeric sanity checks as well? |
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
added, it is not perfect this way, but I would say good enough, what do you think? |
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Show resolved
Hide resolved
…tor/api/reconciler/PrimaryUpdateAndCacheUtils.java Co-authored-by: Steven Hawkins <shawkins@redhat.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Outdated
Show resolved
Hide resolved
…tor/api/reconciler/PrimaryUpdateAndCacheUtils.java Co-authored-by: Steven Hawkins <shawkins@redhat.com>
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Outdated
Show resolved
Hide resolved
…tor/api/reconciler/PrimaryUpdateAndCacheUtils.java Co-authored-by: Steven Hawkins <shawkins@redhat.com>
| if (v2Length == 0) { | ||
| throw new IllegalStateException("Resource version (2) is empty"); | ||
| } | ||
| var maxLength = Math.max(v1Length, v2Length); |
There was a problem hiding this comment.
Assuming we don't have leading zeros, would it make sense to compare the lenghts upfront?
In general there would be not a bug win for that, but might simplify things.
I'm pretty sure etcd does not generate versions with leading zeros
There was a problem hiding this comment.
( I mean I could leave with that assumption :) )
There was a problem hiding this comment.
Assuming we don't have leading zeros, would it make sense to compare the lenghts upfront?
That also short circuits the check for non-digit characters. So if you want to go with that approach, then just revert to the initial state of the pr - and consider putting the resource version validation elsewhere if you want a sanity check.
There was a problem hiding this comment.
For now reverted for sake of simplicity / readability, but we can discuss on next community meeting, and maybe decide with others which one to take.
There was a problem hiding this comment.
But added the validation you proposed
There was a problem hiding this comment.
It's probably confusing to have the other sanity checks in there - I'd vote to either fully validate or not at all.
I would add that the performance difference between fully validating and partially validating should be minimal in the long run. As the api server is up longer, the larger resource versions will get, and the less likely it will be for there to be a difference in the length.
2 participants
based on: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/5504-comparable-resource-version#helper-function
This also seems to be significantly faster also in java, even in worst case scenario.