Refactoring code to address comments

Signed-off-by: Martin Gaievski <[email protected]>
opensearch-project · Jul 6, 2022 · 24dfcdb · 24dfcdb
1 parent 6ad1bda
commit 24dfcdb
Show file tree

Hide file tree

Showing 6 changed files with 30 additions and 37 deletions.
diff --git a/server/src/main/java/org/opensearch/index/codec/KnnVectorFormatFactory.java b/server/src/main/java/org/opensearch/index/codec/KnnVectorFormatFactory.java
@@ -8,7 +8,6 @@
 import org.apache.lucene.codecs.KnnVectorsFormat;
 import org.apache.lucene.codecs.lucene92.Lucene92Codec;
 import org.apache.lucene.codecs.lucene92.Lucene92HnswVectorsFormat;
-import org.opensearch.index.mapper.DenseVectorFieldMapper;
 import org.opensearch.index.mapper.KnnAlgorithmContext;
 import org.opensearch.index.mapper.MappedFieldType;
 import org.opensearch.index.mapper.MapperService;
@@ -54,10 +53,7 @@ public KnnVectorsFormat create(final String field) {
     }
 
     private boolean isDenseVectorFieldType(final MappedFieldType mappedFieldType) {
-        if (mappedFieldType instanceof DenseVectorFieldMapper.DenseVectorFieldType) {
-            return true;
-        }
-        return false;
+        return mappedFieldType instanceof DenseVectorFieldType;
     }
 
     private int getIntegerParam(Map<String, Object> methodParams, String name) {

diff --git a/server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java
@@ -41,7 +41,7 @@ public final class DenseVectorFieldMapper extends FieldMapper {
     /**
      * Define the max dimension a knn_vector mapping can have.
      */
-    public static final int MAX_DIMENSION = 1024;
+    private static final int MAX_DIMENSION = 1024;
 
     private static DenseVectorFieldMapper toType(FieldMapper in) {
         return (DenseVectorFieldMapper) in;

diff --git a/server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java b/server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java
@@ -25,14 +25,14 @@ public class KnnAlgorithmContext implements ToXContentFragment, Writeable {
     private static final String PARAMETERS = "parameters";
     private static final String NAME = "name";
 
+    private static final int MAX_NUMBER_OF_ALGORITHM_PARAMETERS = 50;
+
     private final Method method;
     private final Map<String, Object> parameters;
 
-    private static final int MAX_NUMBER_OF_ALGORITHM_PARAMETERS = 50;
-
     public KnnAlgorithmContext(Method method, Map<String, Object> parameters) {
-        this.method = method;
-        this.parameters = parameters;
+        this.method = Objects.requireNonNull(method, "[method] for knn algorithm context cannot be null");
+        this.parameters = Objects.requireNonNull(parameters, "[parameters] for knn algorithm context cannot be null");
     }
 
     public Method getMethod() {
@@ -78,9 +78,7 @@ public static KnnAlgorithmContext parse(Object in) {
                 parameters = ((Map<String, Object>) value).entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> {
                     Object v = e.getValue();
                     if (v instanceof Map) {
-                        throw new MapperParsingException(
-                            String.format(Locale.ROOT, "Unable to parse parameter [%s] for [algorithm]", e.getValue())
-                        );
+                        throw new MapperParsingException(String.format(Locale.ROOT, "Unable to parse parameter [%s] for [algorithm]", v));
                     }
                     return v;
                 }));
@@ -105,19 +103,17 @@ public static KnnAlgorithmContext parse(Object in) {
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.field(NAME, method.name());
         if (parameters == null) {
-            builder.field(PARAMETERS, (String) null);
-        } else {
-            builder.startObject(PARAMETERS);
-            parameters.forEach((key, value) -> {
-                try {
-                    builder.field(key, value);
-                } catch (IOException ioe) {
-                    throw new RuntimeException("Unable to generate xcontent for method component");
-                }
-
-            });
-            builder.endObject();
+            return builder.field(PARAMETERS, (String) null);
         }
+        builder.startObject(PARAMETERS);
+        parameters.forEach((key, value) -> {
+            try {
+                builder.field(key, value);
+            } catch (IOException ioe) {
+                throw new RuntimeException("Unable to generate xcontent for method component");
+            }
+        });
+        builder.endObject();
         return builder;
     }
 

diff --git a/server/src/main/java/org/opensearch/index/mapper/KnnContext.java b/server/src/main/java/org/opensearch/index/mapper/KnnContext.java
@@ -22,13 +22,14 @@
  */
 public final class KnnContext implements ToXContentFragment, Writeable {
 
-    private final Metric metric;
-    private final KnnAlgorithmContext knnAlgorithmContext;
     private static final String KNN_METRIC_NAME = "metric";
     private static final String ALGORITHM = "algorithm";
 
+    private final Metric metric;
+    private final KnnAlgorithmContext knnAlgorithmContext;
+
     KnnContext(final Metric metric, final KnnAlgorithmContext knnAlgorithmContext) {
-        this.metric = metric;
+        this.metric = Objects.requireNonNull(metric, "[metric] for knn context cannot be null");
         this.knnAlgorithmContext = knnAlgorithmContext;
     }
 

diff --git a/server/src/main/java/org/opensearch/index/translog/InternalTranslogManager.java b/server/src/main/java/org/opensearch/index/translog/InternalTranslogManager.java
@@ -285,9 +285,9 @@ public void ensureCanFlush() {
 
     /**
      * Reads operations from the translog
-     * @param location
+     * @param location the location in the translog
      * @return the translog operation
-     * @throws IOException
+     * @throws IOException if an {@link IOException} occurs while executing method
      */
     @Override
     public Translog.Operation readOperation(Translog.Location location) throws IOException {
@@ -296,9 +296,9 @@ public Translog.Operation readOperation(Translog.Location location) throws IOExc
 
     /**
      * Adds an operation to the translog
-     * @param operation
+     * @param operation the operation in the translog
      * @return the location in the translog
-     * @throws IOException
+     * @throws IOException if an {@link IOException} occurs while executing method
      */
     @Override
     public Translog.Location add(Translog.Operation operation) throws IOException {
@@ -396,8 +396,8 @@ public String getTranslogUUID() {
 
     /**
      *
-     * @param localCheckpointOfLastCommit
-     * @param flushThreshold
+     * @param localCheckpointOfLastCommit the localCheckpoint of last commit in the translog
+     * @param flushThreshold the flush threshold in the translog
      * @return if the translog should be flushed
      */
     public boolean shouldPeriodicallyFlush(long localCheckpointOfLastCommit, long flushThreshold) {

diff --git a/server/src/main/java/org/opensearch/index/translog/TranslogManager.java b/server/src/main/java/org/opensearch/index/translog/TranslogManager.java
@@ -98,15 +98,15 @@ public interface TranslogManager {
      * Reads operations for the translog
      * @param location the location in the translog
      * @return the translog operation
-     * @throws IOException
+     * @throws IOException if an {@link IOException} occurs while executing method
      */
     Translog.Operation readOperation(Translog.Location location) throws IOException;
 
     /**
      * Adds an operation to the translog
-     * @param operation
+     * @param operation translog operation
      * @return the location in the translog
-     * @throws IOException
+     * @throws IOException if an {@link IOException} occurs while executing method
      */
     Translog.Location add(Translog.Operation operation) throws IOException;