Skip to content

Commit aa89c9d

Browse files
Swiddisclaude
Swiddis
and
claude
committed
Address CodeRabbit PR feedback: fix comment, add tests, handle edge case
1. Fixed contradictory comment in ResourceMonitor.isHealthyImpl() that claimed to delegate to old behavior but actually throws exception 2. Added unit test for ResourceMonitor default exception path 3. Created comprehensive unit tests for ResourceStatus class covering: - Factory methods (healthy/unhealthy) for all ResourceType values - getFormattedDescription() with and without metrics - Byte formatting boundary conditions (<1KB, 1KB, 1MB, 1GB) - Edge cases (zero usage, usage equals limit, invalid limit) 4. Fixed getFormattedDescription() to handle maxLimit <= 0 gracefully by treating it as 0 and omitting percentage calculation Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
1 parent dc2a32c commit aa89c9d

4 files changed

Lines changed: 244 additions & 2 deletions

File tree

‎core/src/main/java/org/opensearch/sql/monitor/ResourceMonitor.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ public ResourceStatus getStatus() {
4545
* @return true for healthy, otherwise false.
4646
*/
4747
protected boolean isHealthyImpl() {
48-
// Default: delegate to old isHealthy() behavior
49-
// This prevents infinite recursion for old implementations
48+
// Subclass must override either getStatus() or isHealthyImpl()
5049
throw new UnsupportedOperationException(
5150
"Subclass must override either getStatus() or isHealthyImpl()");
5251
}

‎core/src/main/java/org/opensearch/sql/monitor/ResourceStatus.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ public static ResourceStatus unhealthy(
8383
*/
8484
public String getFormattedDescription() {
8585
if (currentUsage != null && maxLimit != null) {
86+
if (maxLimit <= 0) {
87+
// Treat invalid limit as 0, don't compute percentage
88+
return String.format(
89+
"%s (current: %s, limit: %s)", description, formatBytes(currentUsage), formatBytes(0));
90+
}
8691
double percentage = (double) currentUsage / maxLimit * 100;
8792
return String.format(
8893
"%s (current: %s, limit: %s, usage: %.1f%%)",

‎core/src/test/java/org/opensearch/sql/monitor/ResourceMonitorTest.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.monitor;
7+
8+
import static org.junit.jupiter.api.Assertions.assertThrows;
9+
10+
import org.junit.jupiter.api.Test;
11+
12+
class ResourceMonitorTest {
13+
14+
@Test
15+
void testDefaultImplementationThrowsException() {
16+
// Create a minimal subclass that doesn't override getStatus() or isHealthyImpl()
17+
ResourceMonitor monitor = new ResourceMonitor() {
18+
// Intentionally empty - doesn't override anything
19+
};
20+
21+
// Attempting to use the default path should throw UnsupportedOperationException
22+
assertThrows(UnsupportedOperationException.class, monitor::isHealthy);
23+
}
24+
}

‎core/src/test/java/org/opensearch/sql/monitor/ResourceStatusTest.java‎

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.monitor;
7+
8+
import static org.junit.jupiter.api.Assertions.assertEquals;
9+
import static org.junit.jupiter.api.Assertions.assertFalse;
10+
import static org.junit.jupiter.api.Assertions.assertNotNull;
11+
import static org.junit.jupiter.api.Assertions.assertTrue;
12+
13+
import org.junit.jupiter.api.Test;
14+
import org.opensearch.sql.monitor.ResourceStatus.ResourceType;
15+
16+
class ResourceStatusTest {
17+
18+
@Test
19+
void testHealthyFactoryMethod() {
20+
// Test healthy() for each ResourceType
21+
ResourceStatus memoryStatus = ResourceStatus.healthy(ResourceType.MEMORY);
22+
assertTrue(memoryStatus.isHealthy());
23+
assertEquals(ResourceType.MEMORY, memoryStatus.getType());
24+
assertEquals("MEMORY resources are healthy", memoryStatus.getDescription());
25+
26+
ResourceStatus cpuStatus = ResourceStatus.healthy(ResourceType.CPU);
27+
assertTrue(cpuStatus.isHealthy());
28+
assertEquals(ResourceType.CPU, cpuStatus.getType());
29+
assertEquals("CPU resources are healthy", cpuStatus.getDescription());
30+
31+
ResourceStatus diskStatus = ResourceStatus.healthy(ResourceType.DISK);
32+
assertTrue(diskStatus.isHealthy());
33+
assertEquals(ResourceType.DISK, diskStatus.getType());
34+
assertEquals("DISK resources are healthy", diskStatus.getDescription());
35+
36+
ResourceStatus otherStatus = ResourceStatus.healthy(ResourceType.OTHER);
37+
assertTrue(otherStatus.isHealthy());
38+
assertEquals(ResourceType.OTHER, otherStatus.getType());
39+
assertEquals("OTHER resources are healthy", otherStatus.getDescription());
40+
}
41+
42+
@Test
43+
void testUnhealthyFactoryMethod() {
44+
// Test unhealthy() for each ResourceType
45+
ResourceStatus memoryStatus =
46+
ResourceStatus.unhealthy(ResourceType.MEMORY, 1024, 2048, "Memory limit exceeded");
47+
assertFalse(memoryStatus.isHealthy());
48+
assertEquals(ResourceType.MEMORY, memoryStatus.getType());
49+
assertEquals("Memory limit exceeded", memoryStatus.getDescription());
50+
assertEquals(1024L, memoryStatus.getCurrentUsage());
51+
assertEquals(2048L, memoryStatus.getMaxLimit());
52+
53+
ResourceStatus cpuStatus =
54+
ResourceStatus.unhealthy(ResourceType.CPU, 95, 100, "CPU usage too high");
55+
assertFalse(cpuStatus.isHealthy());
56+
assertEquals(ResourceType.CPU, cpuStatus.getType());
57+
assertEquals("CPU usage too high", cpuStatus.getDescription());
58+
59+
ResourceStatus diskStatus =
60+
ResourceStatus.unhealthy(ResourceType.DISK, 900, 1000, "Disk space low");
61+
assertFalse(diskStatus.isHealthy());
62+
assertEquals(ResourceType.DISK, diskStatus.getType());
63+
assertEquals("Disk space low", diskStatus.getDescription());
64+
65+
ResourceStatus otherStatus =
66+
ResourceStatus.unhealthy(ResourceType.OTHER, 50, 100, "Other resource issue");
67+
assertFalse(otherStatus.isHealthy());
68+
assertEquals(ResourceType.OTHER, otherStatus.getType());
69+
assertEquals("Other resource issue", otherStatus.getDescription());
70+
}
71+
72+
@Test
73+
void testGetFormattedDescriptionWithMetrics() {
74+
// Test with metrics set (using values that will format as GB)
75+
ResourceStatus status =
76+
ResourceStatus.unhealthy(
77+
ResourceType.MEMORY, 1536L * 1024 * 1024, 2048L * 1024 * 1024, "Memory usage high");
78+
79+
String formatted = status.getFormattedDescription();
80+
assertNotNull(formatted);
81+
assertTrue(formatted.contains("Memory usage high"));
82+
assertTrue(formatted.contains("current:"));
83+
assertTrue(formatted.contains("limit:"));
84+
assertTrue(formatted.contains("usage:"));
85+
assertTrue(formatted.contains("75.0%")); // 1536/2048 = 0.75
86+
assertTrue(formatted.contains("1.5GB")); // 1536 MB = 1.5 GB
87+
assertTrue(formatted.contains("2.0GB")); // 2048 MB = 2.0 GB
88+
}
89+
90+
@Test
91+
void testGetFormattedDescriptionWithoutMetrics() {
92+
// Test with null metrics
93+
ResourceStatus status =
94+
ResourceStatus.builder()
95+
.healthy(false)
96+
.type(ResourceType.MEMORY)
97+
.description("Memory issue detected")
98+
.build();
99+
100+
String formatted = status.getFormattedDescription();
101+
assertEquals("Memory issue detected", formatted);
102+
}
103+
104+
@Test
105+
void testByteFormattingLessThan1KB() {
106+
// Test < 1KB
107+
ResourceStatus status = ResourceStatus.unhealthy(ResourceType.MEMORY, 512, 1024, "Low memory");
108+
109+
String formatted = status.getFormattedDescription();
110+
assertTrue(formatted.contains("512B"));
111+
assertTrue(formatted.contains("1.0KB"));
112+
}
113+
114+
@Test
115+
void testByteFormattingExactly1KB() {
116+
// Test exactly 1KB
117+
ResourceStatus status =
118+
ResourceStatus.unhealthy(ResourceType.MEMORY, 1024, 2048, "Memory warning");
119+
120+
String formatted = status.getFormattedDescription();
121+
assertTrue(formatted.contains("1.0KB"));
122+
assertTrue(formatted.contains("2.0KB"));
123+
}
124+
125+
@Test
126+
void testByteFormattingExactly1MB() {
127+
// Test exactly 1MB
128+
long oneMB = 1024 * 1024;
129+
ResourceStatus status =
130+
ResourceStatus.unhealthy(ResourceType.MEMORY, oneMB, oneMB * 2, "Memory warning");
131+
132+
String formatted = status.getFormattedDescription();
133+
assertTrue(formatted.contains("1.0MB"));
134+
assertTrue(formatted.contains("2.0MB"));
135+
}
136+
137+
@Test
138+
void testByteFormattingExactly1GB() {
139+
// Test exactly 1GB
140+
long oneGB = 1024L * 1024 * 1024;
141+
ResourceStatus status =
142+
ResourceStatus.unhealthy(ResourceType.MEMORY, oneGB, oneGB * 2, "Memory warning");
143+
144+
String formatted = status.getFormattedDescription();
145+
assertTrue(formatted.contains("1.0GB"));
146+
assertTrue(formatted.contains("2.0GB"));
147+
}
148+
149+
@Test
150+
void testByteFormattingZeroUsage() {
151+
// Test with 0 usage
152+
ResourceStatus status = ResourceStatus.unhealthy(ResourceType.MEMORY, 0, 1024, "Zero usage");
153+
154+
String formatted = status.getFormattedDescription();
155+
assertTrue(formatted.contains("0B"));
156+
assertTrue(formatted.contains("0.0%"));
157+
}
158+
159+
@Test
160+
void testByteFormattingUsageEqualsLimit() {
161+
// Test when currentUsage == maxLimit
162+
ResourceStatus status = ResourceStatus.unhealthy(ResourceType.MEMORY, 2048, 2048, "At limit");
163+
164+
String formatted = status.getFormattedDescription();
165+
assertTrue(formatted.contains("2.0KB"));
166+
assertTrue(formatted.contains("100.0%"));
167+
}
168+
169+
@Test
170+
void testByteFormattingMultipleSizes() {
171+
// Test various sizes to ensure proper formatting
172+
// Small bytes
173+
ResourceStatus status1 = ResourceStatus.unhealthy(ResourceType.MEMORY, 100, 1000, "Test");
174+
assertTrue(status1.getFormattedDescription().contains("100B"));
175+
176+
// KB range
177+
ResourceStatus status2 =
178+
ResourceStatus.unhealthy(ResourceType.MEMORY, 500 * 1024, 1000 * 1024, "Test");
179+
assertTrue(status2.getFormattedDescription().contains("500.0KB"));
180+
181+
// MB range
182+
ResourceStatus status3 =
183+
ResourceStatus.unhealthy(
184+
ResourceType.MEMORY, 500L * 1024 * 1024, 1000L * 1024 * 1024, "Test");
185+
assertTrue(status3.getFormattedDescription().contains("500.0MB"));
186+
187+
// GB range
188+
ResourceStatus status4 =
189+
ResourceStatus.unhealthy(
190+
ResourceType.MEMORY, 5L * 1024 * 1024 * 1024, 10L * 1024 * 1024 * 1024, "Test");
191+
assertTrue(status4.getFormattedDescription().contains("5.0GB"));
192+
}
193+
194+
@Test
195+
void testMaxLimitZeroOrNegative() {
196+
// Test with maxLimit = 0 (should not compute percentage)
197+
ResourceStatus status1 =
198+
ResourceStatus.unhealthy(ResourceType.MEMORY, 1024, 0, "Invalid limit");
199+
String formatted1 = status1.getFormattedDescription();
200+
assertTrue(formatted1.contains("Invalid limit"));
201+
assertTrue(formatted1.contains("current:"));
202+
assertTrue(formatted1.contains("limit:"));
203+
assertFalse(formatted1.contains("%")); // Should not contain percentage
204+
205+
// Test with maxLimit < 0 (should not compute percentage)
206+
ResourceStatus status2 =
207+
ResourceStatus.unhealthy(ResourceType.MEMORY, 1024, -100, "Negative limit");
208+
String formatted2 = status2.getFormattedDescription();
209+
assertTrue(formatted2.contains("Negative limit"));
210+
assertTrue(formatted2.contains("current:"));
211+
assertTrue(formatted2.contains("limit:"));
212+
assertFalse(formatted2.contains("%")); // Should not contain percentage
213+
}
214+
}

0 commit comments

Comments
 (0)