From f37597720a6161b6d4983652eaea553431802efe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Wed, 19 Jun 2024 16:42:22 +0200 Subject: [PATCH] The value of fileCacheUtilized should use standard init value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In FsInfo.Path object the value of fileCacheUtilized is now initialized like any other fields, that means it is -1, which means the value hasn't been populated yet. To make this possible it was necessary to fix FsProbeTest and use a real FileCache object to enable internal logic to handle initialization and safeguarding of the value properly. Signed-off-by: Lukáš Vlček --- .../org/opensearch/monitor/fs/FsInfo.java | 2 +- .../org/opensearch/monitor/fs/FsProbe.java | 5 ++++- .../opensearch/monitor/fs/FsProbeTests.java | 22 ++++++++++++++++++- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java index 607c56591b5c8..07572c4c3018b 100644 --- a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java +++ b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java @@ -75,7 +75,7 @@ public static class Path implements Writeable, ToXContentObject { long free = -1; long available = -1; long fileCacheReserved = -1; - long fileCacheUtilized = 0; + long fileCacheUtilized = -1; public Path() {} diff --git a/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java b/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java index f4731a4a34373..7a18fc564982a 100644 --- a/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java +++ b/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java @@ -78,7 +78,7 @@ public FsInfo stats(FsInfo previous) throws IOException { FsInfo.Path[] paths = new FsInfo.Path[dataLocations.length]; for (int i = 0; i < dataLocations.length; i++) { paths[i] = getFSInfo(dataLocations[i]); - if (fileCache != null && dataLocations[i].fileCacheReservedSize != ByteSizeValue.ZERO) { + if (fileCache != null && dataLocations[i].fileCacheReservedSize.compareTo(ByteSizeValue.ZERO) >= 0) { paths[i].fileCacheReserved = adjustForHugeFilesystems(dataLocations[i].fileCacheReservedSize.getBytes()); paths[i].fileCacheUtilized = adjustForHugeFilesystems(fileCache.usage().usage()); paths[i].available -= (paths[i].fileCacheReserved - paths[i].fileCacheUtilized); @@ -206,6 +206,9 @@ public static FsInfo.Path getFSInfo(NodePath nodePath) throws IOException { fsPath.free = adjustForHugeFilesystems(nodePath.fileStore.getUnallocatedSpace()); fsPath.available = adjustForHugeFilesystems(nodePath.fileStore.getUsableSpace()); fsPath.fileCacheReserved = adjustForHugeFilesystems(nodePath.fileCacheReservedSize.getBytes()); + // fsPath.fileCacheUtilized = adjustForHugeFilesystems(...); + // We can not do this ^^ here because information about utilization of file cache is hold by FileCache + // which is not accessible here and since this method is static we can not assume relevant context. fsPath.type = nodePath.fileStore.type(); fsPath.mount = nodePath.fileStore.toString(); return fsPath; diff --git a/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java b/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java index 8223184294992..68f1be070a3ef 100644 --- a/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java +++ b/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java @@ -71,7 +71,17 @@ public class FsProbeTests extends OpenSearchTestCase { public void testFsInfo() throws IOException { try (NodeEnvironment env = newNodeEnvironment()) { - FsProbe probe = new FsProbe(env, null); + // Question: Shall we expose a public method in FileCacheTests to enable creation of FileCache + // so that it can be used by other testing classes? + int CONCURRENCY_LEVEL = 16; // not important + int CAPACITY = 1 * 1024; // not important + FileCache fileCache = FileCacheFactory.createConcurrentLRUFileCache( + CAPACITY, + CONCURRENCY_LEVEL, + new NoopCircuitBreaker(CircuitBreaker.REQUEST) + ); + // We need to pass a real FileCache object to FsProbe ctor to have it safeguard "path.fileCacheUtilized" values properly! + FsProbe probe = new FsProbe(env, fileCache); FsInfo stats = probe.stats(null); assertNotNull(stats); @@ -110,6 +120,16 @@ public void testFsInfo() throws IOException { assertThat(total.free, greaterThan(0L)); assertThat(total.available, greaterThan(0L)); + // The convention for "total" Path object is that some fields are not set + // which means they will not be included in output of toXContent method. + assertNull(total.path); + assertNull(total.mount); + assertNull(total.type); + + // Total file cache (sum over all "paths"): + assertEquals(total.getFileCacheReserved().getBytes(), 0); + assertEquals(total.getFileCacheUtilized().getBytes(), 0); + for (FsInfo.Path path : stats) { assertNotNull(path); assertThat(path.getPath(), is(not(emptyOrNullString())));