Address Review Comments

Signed-off-by: Naveen Tatikonda <[email protected]>

CHANGELOG.md

@@ -14,7 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 
 ## [Unreleased 2.x](https://github.com/opensearch-project/k-NN/compare/2.9...2.x)
 ### Features
-* Add Clear Cache API ([#740](https://github.com/opensearch-project/k-NN/pull/740))
+* Add Clear Cache API [#740](https://github.com/opensearch-project/k-NN/pull/740)
 ### Enhancements
 * Enabled the IVF algorithm to work with Filters of K-NN Query. [#1013](https://github.com/opensearch-project/k-NN/pull/1013)
 ### Bug Fixes

src/main/java/org/opensearch/knn/index/KNNIndexShard.java

@@ -5,9 +5,8 @@
 
 package org.opensearch.knn.index;
 
+import lombok.extern.log4j.Log4j2;
 import org.apache.lucene.index.FieldInfo;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.apache.lucene.index.FilterLeafReader;
 import org.apache.lucene.index.IndexReader;
 import org.apache.lucene.index.LeafReaderContext;
@@ -40,13 +39,12 @@
 /**
  * KNNIndexShard wraps IndexShard and adds methods to perform k-NN related operations against the shard
  */
+@Log4j2
 public class KNNIndexShard {
     private IndexShard indexShard;
     private NativeMemoryCacheManager nativeMemoryCacheManager;
     private static final String INDEX_SHARD_CLEAR_CACHE_SEARCHER = "knn-clear-cache";
 
-    private static Logger logger = LogManager.getLogger(KNNIndexShard.class);
-
     /**
      * Constructor to generate KNNIndexShard. We do not perform validation that the index the shard is from
      * is in fact a k-NN Index (index.knn = true). This may make sense to add later, but for now the operations for
@@ -83,7 +81,7 @@ public String getIndexName() {
      * @throws IOException Thrown when getting the HNSW Paths to be loaded in
      */
     public void warmup() throws IOException {
-        logger.info("[KNN] Warming up index: " + getIndexName());
+        log.info("[KNN] Warming up index: " + getIndexName());
         try (Engine.Searcher searcher = indexShard.acquireSearcher("knn-warmup")) {
             getAllEnginePaths(searcher.getIndexReader()).forEach((key, value) -> {
                 try {
@@ -117,12 +115,11 @@ public void clearCache() {
         if (indexAllocationOptional.isPresent()) {
             indexAllocation = indexAllocationOptional.get();
             indexAllocation.writeLock();
-            logger.info("[KNN] Evicting index from cache: [{}]", indexName);
+            log.info("[KNN] Evicting index from cache: [{}]", indexName);
             try (Engine.Searcher searcher = indexShard.acquireSearcher(INDEX_SHARD_CLEAR_CACHE_SEARCHER)) {
                 getAllEnginePaths(searcher.getIndexReader()).forEach((key, value) -> nativeMemoryCacheManager.invalidate(key));
             } catch (IOException ex) {
-                logger.error("[KNN] Failed to evict index from cache: [{}]", indexName);
-                logger.error(ex.getMessage());
+                log.error("[KNN] Failed to evict index from cache: [{}]", indexName, ex);
                 throw new RuntimeException(ex);
             } finally {
                 indexAllocation.writeUnlock();

src/main/java/org/opensearch/knn/plugin/rest/RestClearCacheHandler.java

@@ -7,8 +7,7 @@
 
 import com.google.common.collect.ImmutableList;
 import lombok.AllArgsConstructor;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
+import lombok.extern.log4j.Log4j2;
 import org.opensearch.client.node.NodeClient;
 import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
 import org.opensearch.cluster.service.ClusterService;
@@ -35,9 +34,8 @@
  * RestHandler for k-NN Clear Cache API. API provides the ability for a user to evict those indices from Cache.
  */
 @AllArgsConstructor
+@Log4j2
 public class RestClearCacheHandler extends BaseRestHandler {
-    private static final Logger logger = LogManager.getLogger(RestClearCacheHandler.class);
-
     private static final String INDEX = "index";
     public static String NAME = "knn_clear_cache_action";
     private final ClusterService clusterService;
@@ -69,7 +67,7 @@ public List<Route> routes() {
     @Override
     protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) {
         ClearCacheRequest clearCacheRequest = createClearCacheRequest(request);
-        logger.info("[KNN] ClearCache started for the following indices: [{}]", String.join(",", clearCacheRequest.indices()));
+        log.info("[KNN] ClearCache started for the following indices: [{}]", String.join(",", clearCacheRequest.indices()));
         return channel -> client.execute(ClearCacheAction.INSTANCE, clearCacheRequest, new RestToXContentListener<>(channel));
     }
 

src/main/java/org/opensearch/knn/plugin/transport/ClearCacheTransportAction.java

@@ -5,10 +5,7 @@
 
 package org.opensearch.knn.plugin.transport;
 
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.opensearch.action.support.ActionFilters;
-import org.opensearch.core.action.support.DefaultShardOperationFailedException;
 import org.opensearch.action.support.broadcast.node.TransportBroadcastByNodeAction;
 import org.opensearch.cluster.ClusterState;
 import org.opensearch.cluster.block.ClusterBlockException;
@@ -18,6 +15,7 @@
 import org.opensearch.cluster.routing.ShardsIterator;
 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.common.inject.Inject;
+import org.opensearch.core.action.support.DefaultShardOperationFailedException;
 import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.core.index.Index;
 import org.opensearch.index.IndexService;
@@ -40,8 +38,6 @@ public class ClearCacheTransportAction extends TransportBroadcastByNodeAction<
     ClearCacheResponse,
     TransportBroadcastByNodeAction.EmptyResult> {
 
-    public static Logger logger = LogManager.getLogger(ClearCacheTransportAction.class);
-
     private IndicesService indicesService;
 
     /**

src/test/java/org/opensearch/knn/index/KNNIndexShardTests.java

@@ -7,6 +7,7 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import lombok.SneakyThrows;
 import org.opensearch.knn.KNNSingleNodeTestCase;
 import org.opensearch.index.IndexService;
 import org.opensearch.index.engine.Engine;
@@ -153,7 +154,8 @@ public void testGetEnginePaths() {
         included.keySet().forEach(o -> assertTrue(includedFileNames.contains(o)));
     }
 
-    public void testClearCache_emptyIndex() throws IOException {
+    @SneakyThrows
+    public void testClearCache_emptyIndex() {
         IndexService indexService = createKNNIndex(testIndexName);
         createKnnIndexMapping(testIndexName, testFieldName, dimensions);
 
@@ -163,7 +165,8 @@ public void testClearCache_emptyIndex() throws IOException {
         assertNull(NativeMemoryCacheManager.getInstance().getIndicesCacheStats().get(testIndexName));
     }
 
-    public void testClearCache_shardPresentInCache() throws InterruptedException, ExecutionException, IOException {
+    @SneakyThrows
+    public void testClearCache_shardPresentInCache() {
         IndexService indexService = createKNNIndex(testIndexName);
         createKnnIndexMapping(testIndexName, testFieldName, dimensions);
         addKnnDoc(testIndexName, String.valueOf(randomInt()), testFieldName, new Float[] { randomFloat(), randomFloat() });

src/test/java/org/opensearch/knn/plugin/action/RestClearCacheHandlerIT.java

@@ -5,14 +5,14 @@
 
 package org.opensearch.knn.plugin.action;
 
+import lombok.SneakyThrows;
 import org.opensearch.client.Request;
 import org.opensearch.client.ResponseException;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.knn.KNNRestTestCase;
 import org.opensearch.knn.plugin.KNNPlugin;
 import org.opensearch.rest.RestRequest;
 
-import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
 
@@ -27,7 +27,8 @@ public class RestClearCacheHandlerIT extends KNNRestTestCase {
     private static final int DIMENSIONS = 2;
     private static final String ALL_INDICES = "_all";
 
-    public void testNonExistentIndex() throws IOException {
+    @SneakyThrows
+    public void testNonExistentIndex() {
         String nonExistentIndex = "non-existent-index";
 
         String restURI = String.join("/", KNNPlugin.KNN_BASE_URI, CLEAR_CACHE, nonExistentIndex);
@@ -37,7 +38,8 @@ public void testNonExistentIndex() throws IOException {
         assertTrue(ex.getMessage().contains(nonExistentIndex));
     }
 
-    public void testNotKnnIndex() throws IOException {
+    @SneakyThrows
+    public void testNotKnnIndex() {
         String notKNNIndex = "not-knn-index";
         createIndex(notKNNIndex, Settings.EMPTY);
 
@@ -48,7 +50,8 @@ public void testNotKnnIndex() throws IOException {
         assertTrue(ex.getMessage().contains(notKNNIndex));
     }
 
-    public void testClearCacheSingleIndex() throws Exception {
+    @SneakyThrows
+    public void testClearCacheSingleIndex() {
         String testIndex = getTestName().toLowerCase();
         int graphCountBefore = getTotalGraphsInCache();
         createKnnIndex(testIndex, getKNNDefaultIndexSettings(), createKnnIndexMapping(TEST_FIELD, DIMENSIONS));
@@ -62,7 +65,8 @@ public void testClearCacheSingleIndex() throws Exception {
         assertEquals(graphCountBefore, getTotalGraphsInCache());
     }
 
-    public void testClearCacheMultipleIndices() throws Exception {
+    @SneakyThrows
+    public void testClearCacheMultipleIndices() {
         String testIndex1 = getTestName().toLowerCase();
         String testIndex2 = getTestName().toLowerCase() + 1;
         int graphCountBefore = getTotalGraphsInCache();
@@ -81,7 +85,8 @@ public void testClearCacheMultipleIndices() throws Exception {
         assertEquals(graphCountBefore, getTotalGraphsInCache());
     }
 
-    public void testClearCacheMultipleIndicesWithPatterns() throws Exception {
+    @SneakyThrows
+    public void testClearCacheMultipleIndicesWithPatterns() {
         String testIndex1 = getTestName().toLowerCase();
         String testIndex2 = getTestName().toLowerCase() + 1;
         String testIndex3 = "abc" + getTestName().toLowerCase();

src/test/java/org/opensearch/knn/plugin/transport/ClearCacheTransportActionTests.java

@@ -5,6 +5,7 @@
 
 package org.opensearch.knn.plugin.transport;
 
+import lombok.SneakyThrows;
 import org.opensearch.cluster.ClusterName;
 import org.opensearch.cluster.ClusterState;
 import org.opensearch.cluster.block.ClusterBlockException;
@@ -16,9 +17,7 @@
 import org.opensearch.knn.KNNSingleNodeTestCase;
 import org.opensearch.knn.index.memory.NativeMemoryCacheManager;
 
-import java.io.IOException;
 import java.util.EnumSet;
-import java.util.concurrent.ExecutionException;
 
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -27,7 +26,8 @@ public class ClearCacheTransportActionTests extends KNNSingleNodeTestCase {
     private static final String TEST_FIELD = "test-field";
     private static final int DIMENSIONS = 2;
 
-    public void testShardOperation() throws IOException, ExecutionException, InterruptedException {
+    @SneakyThrows
+    public void testShardOperation() {
         String testIndex = getTestName().toLowerCase();
         KNNWarmupRequest knnWarmupRequest = new KNNWarmupRequest(testIndex);
         KNNWarmupTransportAction knnWarmupTransportAction = node().injector().getInstance(KNNWarmupTransportAction.class);
@@ -47,7 +47,8 @@ public void testShardOperation() throws IOException, ExecutionException, Interru
         assertEquals(0, NativeMemoryCacheManager.getInstance().getIndicesCacheStats().size());
     }
 
-    public void testShards() throws InterruptedException, ExecutionException, IOException {
+    @SneakyThrows
+    public void testShards() {
         String testIndex = getTestName().toLowerCase();
         ClusterService clusterService = node().injector().getInstance(ClusterService.class);
         ClearCacheTransportAction clearCacheTransportAction = node().injector().getInstance(ClearCacheTransportAction.class);