Skip to content

Commit a870ac7

Browse files
csviricsviri
committed
Compare functions without validation
Signed-off-by: Attila Mészáros <[email protected]>
1 parent ff171e7 commit a870ac7

File tree

4 files changed

+166
-30
lines changed

4 files changed

+166
-30
lines changed

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ReconcileUtils.java‎

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -921,19 +921,76 @@ public static <P extends HasMetadata> P addFinalizerWithSSA(
921921

922922
/**
923923
* Compares resource versions of two resources. This is a convenience method that extracts the
924-
* resource versions from the metadata and delegates to {@link #compareResourceVersions(String,
925-
* String)}.
924+
* resource versions from the metadata and delegates to {@link
925+
* #validateAndCompareResourceVersions(String, String)}.
926926
*
927927
* @param h1 first resource
928928
* @param h2 second resource
929929
* @return negative if h1 is older, zero if equal, positive if h1 is newer
930930
* @throws NonComparableResourceVersionException if either resource version is invalid
931931
*/
932+
public static int validateAndCompareResourceVersions(HasMetadata h1, HasMetadata h2) {
933+
return validateAndCompareResourceVersions(
934+
h1.getMetadata().getResourceVersion(), h2.getMetadata().getResourceVersion());
935+
}
936+
937+
/**
938+
* Compares the resource versions of two Kubernetes resources.
939+
*
940+
* <p>This method extracts the resource versions from the metadata of both resources and delegates
941+
* to {@link #compareResourceVersions(String, String)} for the actual comparison.
942+
*
943+
* @param h1 the first resource to compare
944+
* @param h2 the second resource to compare
945+
* @return a negative integer if h1's version is less than h2's version, zero if they are equal,
946+
* or a positive integer if h1's version is greater than h2's version
947+
* @see #compareResourceVersions(String, String)
948+
*/
932949
public static int compareResourceVersions(HasMetadata h1, HasMetadata h2) {
933950
return compareResourceVersions(
934951
h1.getMetadata().getResourceVersion(), h2.getMetadata().getResourceVersion());
935952
}
936953

954+
/**
955+
* Compares two resource version strings using a length-first, then lexicographic comparison
956+
* algorithm.
957+
*
958+
* <p>The comparison is performed in two steps:
959+
*
960+
* <ol>
961+
* <li>First, compare the lengths of the version strings. A longer version string is considered
962+
* greater than a shorter one. This works correctly for numeric versions because larger
963+
* numbers have more digits (e.g., "100" > "99").
964+
* <li>If the lengths are equal, perform a character-by-character lexicographic comparison until
965+
* a difference is found.
966+
* </ol>
967+
*
968+
* <p>This algorithm is more efficient than parsing the versions as numbers, especially for
969+
* Kubernetes resource versions which are typically monotonically increasing numeric strings.
970+
*
971+
* <p><strong>Note:</strong> This method does not validate that the input strings are numeric. For
972+
* validated numeric comparison, use {@link #validateAndCompareResourceVersions(String, String)}.
973+
*
974+
* @param v1 the first resource version string
975+
* @param v2 the second resource version string
976+
* @return a negative integer if v1 is less than v2, zero if they are equal, or a positive integer
977+
* if v1 is greater than v2
978+
* @see #validateAndCompareResourceVersions(String, String)
979+
*/
980+
public static int compareResourceVersions(String v1, String v2) {
981+
int comparison = v1.length() - v2.length();
982+
if (comparison != 0) {
983+
return comparison;
984+
}
985+
for (int i = 0; i < v2.length(); i++) {
986+
int comp = v1.charAt(i) - v2.charAt(i);
987+
if (comp != 0) {
988+
return comp;
989+
}
990+
}
991+
return 0;
992+
}
993+
937994
/**
938995
* Compares two Kubernetes resource versions numerically. Kubernetes resource versions are
939996
* expected to be numeric strings that increase monotonically. This method assumes both versions
@@ -945,7 +1002,7 @@ public static int compareResourceVersions(HasMetadata h1, HasMetadata h2) {
9451002
* @throws NonComparableResourceVersionException if either resource version is empty, has leading
9461003
* zeros, or contains non-numeric characters
9471004
*/
948-
public static int compareResourceVersions(String v1, String v2) {
1005+
public static int validateAndCompareResourceVersions(String v1, String v2) {
9491006
int v1Length = validateResourceVersion(v1);
9501007
int v2Length = validateResourceVersion(v2);
9511008
int comparison = v1Length - v2Length;

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,10 @@ public Optional<R> get(ResourceID resourceID) {
176176
Optional<R> resource = temporaryResourceCache.getResourceFromCache(resourceID);
177177
if (comparableResourceVersions
178178
&& resource.isPresent()
179-
&& res.filter(r -> ReconcileUtils.compareResourceVersions(r, resource.orElseThrow()) > 0)
179+
&& res.filter(
180+
r ->
181+
ReconcileUtils.validateAndCompareResourceVersions(r, resource.orElseThrow())
182+
> 0)
180183
.isEmpty()) {
181184
log.debug("Latest resource found in temporary cache for Resource ID: {}", resourceID);
182185
return resource;

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,6 @@
5353
*/
5454
public class TemporaryResourceCache<T extends HasMetadata> {
5555

56-
// TODO
57-
// requirements:
58-
// - concurrent updates
59-
// - no blocking i/o related updates
60-
// - support non-event filtering
61-
// - handle delete event
62-
6356
private static final Logger log = LoggerFactory.getLogger(TemporaryResourceCache.class);
6457

6558
private final Map<ResourceID, T> cache = new ConcurrentHashMap<>();
@@ -131,7 +124,7 @@ private synchronized boolean onEvent(T resource, boolean unknownState, boolean d
131124
boolean filterEvent = false;
132125
int comp = 0;
133126
if (cached != null) {
134-
comp = ReconcileUtils.compareResourceVersions(resource, cached);
127+
comp = ReconcileUtils.validateAndCompareResourceVersions(resource, cached);
135128
if (comp >= 0 || unknownState) {
136129
cache.remove(resourceId);
137130
// we propagate event only for our update or newer other can be discarded since we know we
@@ -198,7 +191,7 @@ public synchronized void putResource(T newResource) {
198191
var cachedResource = getResourceFromCache(resourceId).orElse(null);
199192

200193
if (cachedResource == null
201-
|| ReconcileUtils.compareResourceVersions(newResource, cachedResource) > 0) {
194+
|| ReconcileUtils.validateAndCompareResourceVersions(newResource, cachedResource) > 0) {
202195
log.debug(
203196
"Temporarily moving ahead to target version {} for resource id: {}",
204197
newResource.getMetadata().getResourceVersion(),

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ReconcileUtilsTest.java‎

Lines changed: 100 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@
2020
import org.slf4j.Logger;
2121
import org.slf4j.LoggerFactory;
2222

23-
import static io.javaoperatorsdk.operator.api.reconciler.ReconcileUtils.compareResourceVersions;
23+
import io.fabric8.kubernetes.api.model.HasMetadata;
24+
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
25+
import io.fabric8.kubernetes.api.model.PodBuilder;
26+
2427
import static org.assertj.core.api.Assertions.assertThat;
2528
import static org.junit.jupiter.api.Assertions.*;
2629

@@ -29,30 +32,110 @@ class ReconcileUtilsTest {
2932
private static final Logger log = LoggerFactory.getLogger(ReconcileUtilsTest.class);
3033

3134
@Test
32-
public void compareResourceVersionsTest() {
33-
assertThat(compareResourceVersions("11", "22")).isNegative();
34-
assertThat(compareResourceVersions("22", "11")).isPositive();
35-
assertThat(compareResourceVersions("1", "1")).isZero();
36-
assertThat(compareResourceVersions("11", "11")).isZero();
37-
assertThat(compareResourceVersions("123", "2")).isPositive();
38-
assertThat(compareResourceVersions("3", "211")).isNegative();
35+
public void validateAndCompareResourceVersionsTest() {
36+
assertThat(ReconcileUtils.validateAndCompareResourceVersions("11", "22")).isNegative();
37+
assertThat(ReconcileUtils.validateAndCompareResourceVersions("22", "11")).isPositive();
38+
assertThat(ReconcileUtils.validateAndCompareResourceVersions("1", "1")).isZero();
39+
assertThat(ReconcileUtils.validateAndCompareResourceVersions("11", "11")).isZero();
40+
assertThat(ReconcileUtils.validateAndCompareResourceVersions("123", "2")).isPositive();
41+
assertThat(ReconcileUtils.validateAndCompareResourceVersions("3", "211")).isNegative();
3942

4043
assertThrows(
41-
NonComparableResourceVersionException.class, () -> compareResourceVersions("aa", "22"));
44+
NonComparableResourceVersionException.class,
45+
() -> ReconcileUtils.validateAndCompareResourceVersions("aa", "22"));
4246
assertThrows(
43-
NonComparableResourceVersionException.class, () -> compareResourceVersions("11", "ba"));
47+
NonComparableResourceVersionException.class,
48+
() -> ReconcileUtils.validateAndCompareResourceVersions("11", "ba"));
4449
assertThrows(
45-
NonComparableResourceVersionException.class, () -> compareResourceVersions("", "22"));
50+
NonComparableResourceVersionException.class,
51+
() -> ReconcileUtils.validateAndCompareResourceVersions("", "22"));
4652
assertThrows(
47-
NonComparableResourceVersionException.class, () -> compareResourceVersions("11", ""));
53+
NonComparableResourceVersionException.class,
54+
() -> ReconcileUtils.validateAndCompareResourceVersions("11", ""));
4855
assertThrows(
49-
NonComparableResourceVersionException.class, () -> compareResourceVersions("01", "123"));
56+
NonComparableResourceVersionException.class,
57+
() -> ReconcileUtils.validateAndCompareResourceVersions("01", "123"));
5058
assertThrows(
51-
NonComparableResourceVersionException.class, () -> compareResourceVersions("123", "01"));
59+
NonComparableResourceVersionException.class,
60+
() -> ReconcileUtils.validateAndCompareResourceVersions("123", "01"));
5261
assertThrows(
53-
NonComparableResourceVersionException.class, () -> compareResourceVersions("3213", "123a"));
62+
NonComparableResourceVersionException.class,
63+
() -> ReconcileUtils.validateAndCompareResourceVersions("3213", "123a"));
5464
assertThrows(
55-
NonComparableResourceVersionException.class, () -> compareResourceVersions("321", "123a"));
65+
NonComparableResourceVersionException.class,
66+
() -> ReconcileUtils.validateAndCompareResourceVersions("321", "123a"));
67+
}
68+
69+
@Test
70+
public void compareResourceVersionsWithStrings() {
71+
// Test equal versions
72+
assertThat(ReconcileUtils.compareResourceVersions("1", "1")).isZero();
73+
assertThat(ReconcileUtils.compareResourceVersions("123", "123")).isZero();
74+
75+
// Test different lengths - shorter version is less than longer version
76+
assertThat(ReconcileUtils.compareResourceVersions("1", "12")).isNegative();
77+
assertThat(ReconcileUtils.compareResourceVersions("12", "1")).isPositive();
78+
assertThat(ReconcileUtils.compareResourceVersions("99", "100")).isNegative();
79+
assertThat(ReconcileUtils.compareResourceVersions("100", "99")).isPositive();
80+
assertThat(ReconcileUtils.compareResourceVersions("9", "100")).isNegative();
81+
assertThat(ReconcileUtils.compareResourceVersions("100", "9")).isPositive();
82+
83+
// Test same length - lexicographic comparison
84+
assertThat(ReconcileUtils.compareResourceVersions("1", "2")).isNegative();
85+
assertThat(ReconcileUtils.compareResourceVersions("2", "1")).isPositive();
86+
assertThat(ReconcileUtils.compareResourceVersions("11", "12")).isNegative();
87+
assertThat(ReconcileUtils.compareResourceVersions("12", "11")).isPositive();
88+
assertThat(ReconcileUtils.compareResourceVersions("99", "100")).isNegative();
89+
assertThat(ReconcileUtils.compareResourceVersions("100", "99")).isPositive();
90+
assertThat(ReconcileUtils.compareResourceVersions("123", "124")).isNegative();
91+
assertThat(ReconcileUtils.compareResourceVersions("124", "123")).isPositive();
92+
93+
// Test with non-numeric strings (algorithm should still work character-wise)
94+
assertThat(ReconcileUtils.compareResourceVersions("a", "b")).isNegative();
95+
assertThat(ReconcileUtils.compareResourceVersions("b", "a")).isPositive();
96+
assertThat(ReconcileUtils.compareResourceVersions("abc", "abd")).isNegative();
97+
assertThat(ReconcileUtils.compareResourceVersions("abd", "abc")).isPositive();
98+
99+
// Test edge cases with larger numbers
100+
assertThat(ReconcileUtils.compareResourceVersions("1234567890", "1234567891")).isNegative();
101+
assertThat(ReconcileUtils.compareResourceVersions("1234567891", "1234567890")).isPositive();
102+
}
103+
104+
@Test
105+
public void compareResourceVersionsWithHasMetadata() {
106+
// Test equal versions
107+
HasMetadata resource1 = createResourceWithVersion("123");
108+
HasMetadata resource2 = createResourceWithVersion("123");
109+
assertThat(ReconcileUtils.compareResourceVersions(resource1, resource2)).isZero();
110+
111+
// Test different lengths
112+
resource1 = createResourceWithVersion("1");
113+
resource2 = createResourceWithVersion("12");
114+
assertThat(ReconcileUtils.compareResourceVersions(resource1, resource2)).isNegative();
115+
assertThat(ReconcileUtils.compareResourceVersions(resource2, resource1)).isPositive();
116+
117+
// Test same length, different values
118+
resource1 = createResourceWithVersion("100");
119+
resource2 = createResourceWithVersion("200");
120+
assertThat(ReconcileUtils.compareResourceVersions(resource1, resource2)).isNegative();
121+
assertThat(ReconcileUtils.compareResourceVersions(resource2, resource1)).isPositive();
122+
123+
// Test realistic Kubernetes resource versions
124+
resource1 = createResourceWithVersion("12345");
125+
resource2 = createResourceWithVersion("12346");
126+
assertThat(ReconcileUtils.compareResourceVersions(resource1, resource2)).isNegative();
127+
assertThat(ReconcileUtils.compareResourceVersions(resource2, resource1)).isPositive();
128+
}
129+
130+
private HasMetadata createResourceWithVersion(String resourceVersion) {
131+
return new PodBuilder()
132+
.withMetadata(
133+
new ObjectMetaBuilder()
134+
.withName("test-pod")
135+
.withNamespace("default")
136+
.withResourceVersion(resourceVersion)
137+
.build())
138+
.build();
56139
}
57140

58141
// naive performance test that compares the work case scenario for the parsing and non-parsing
@@ -63,7 +146,7 @@ public void compareResourcePerformanceTest() {
63146
var execNum = 30000000;
64147
var startTime = System.currentTimeMillis();
65148
for (int i = 0; i < execNum; i++) {
66-
var res = compareResourceVersions("123456788", "123456789");
149+
var res = ReconcileUtils.compareResourceVersions("123456788", "123456789");
67150
}
68151
var dur1 = System.currentTimeMillis() - startTime;
69152
log.info("Duration without parsing: {}", dur1);

0 commit comments

Comments
 (0)