[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #9339: Optimize elasticsearch query performance by using `_mGet` and physical index name

[GitBox <gi...@apache.org>]

kezhenxu94 commented on code in PR #9339:
URL: https://github.com/apache/skywalking/pull/9339#discussion_r919671464


##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/factory/DocumentFactory.java:
##########
@@ -39,6 +40,11 @@ public interface DocumentFactory {
      */
     HttpRequest mget(String index, String type, Iterable<String> ids);
 
+    /**
+     * Returns a request to get multiple documents of {@code indexIds}.
+     */
+    HttpRequest mget(final String type, final Map<String, List<String>> indexIdsGroup);

Review Comment:
   You wrote `indexIds` in JavaDoc and `Group` is ok to remove as `Map` type can indicate that.
   
   ```suggestion
       HttpRequest mget(final String type, final Map<String, List<String>> indexIds);
   ```



##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/TimeSeriesUtils.java:
##########
@@ -80,6 +82,33 @@ public static String[] superDatasetIndexNames(String indexName, long startSecond
         }
     }
 
+    public static String queryIndexName(String tableName,
+                                        long pointOfTB,
+                                        Step step,
+                                        boolean isRecord,
+                                        boolean isSuperDataSet) {
+        if (StringUtil.isBlank(tableName) || pointOfTB <= 0) {
+            throw new IllegalArgumentException(
+                "Arguments [tableName]: " + tableName + " can not be blank and [pointOfTB]: " + pointOfTB + " can not <= 0");
+        }
+        if (isRecord && isSuperDataSet) {
+            return tableName + Const.LINE + compressTimeBucket(pointOfTB / 1000000, SUPER_DATASET_DAY_STEP);
+        } else {
+            switch (step) {
+                case DAY:
+                    return tableName + Const.LINE + compressTimeBucket(pointOfTB, DAY_STEP);
+                case HOUR:
+                    return tableName + Const.LINE + compressTimeBucket(pointOfTB / 100, DAY_STEP);
+                case MINUTE:
+                    return tableName + Const.LINE + compressTimeBucket(pointOfTB / 10000, DAY_STEP);
+                case SECOND:
+                    return tableName + Const.LINE + compressTimeBucket(pointOfTB / 1000000, DAY_STEP);
+            }
+        }

Review Comment:
   ```suggestion
           }
           switch (step) {
               case DAY:
                   return tableName + Const.LINE + compressTimeBucket(pointOfTB, DAY_STEP);
               case HOUR:
                   return tableName + Const.LINE + compressTimeBucket(pointOfTB / 100, DAY_STEP);
               case MINUTE:
                   return tableName + Const.LINE + compressTimeBucket(pointOfTB / 10000, DAY_STEP);
               case SECOND:
                   return tableName + Const.LINE + compressTimeBucket(pointOfTB / 1000000, DAY_STEP);
           }
   ```



##########
oap-server/server-library/library-client/src/main/java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java:
##########
@@ -300,13 +303,33 @@ public boolean existDoc(String indexName, String id) {
         return es.get().documents().exists(indexName, TYPE, id);
     }
 
-    public SearchResponse ids(String indexName, Iterable<String> ids) {
-        indexName = indexNameConverter.apply(indexName);
+
+    /**
+     * @since 9.2.0 Provide to get documents from multi indices by ids.
+     * @param indexIdsGroup key: indexName, value: ids list
+     * @return Documents
+     */
+    public Optional<Documents> ids(Map<String, List<String>> indexIdsGroup) {
+        Map<String, List<String>> map = new HashMap<>();
+        indexIdsGroup.forEach((indexName, ids) -> {
+            map.put(indexNameConverter.apply(indexName), ids);
+        });
+        return es.get().documents().mGet(TYPE, map);
+    }
+
+    /**
+     * Query by ids with index alias. When can not locate the physical index.
+     * @param indexAlias Index alias name
+     * @param ids Query ids
+     * @return SearchResponse
+     */
+    public SearchResponse idsWithIndexAlias(String indexAlias, Iterable<String> ids) {

Review Comment:
   What about just keep the method name `ids` and parameter `index`? This method still can query by both physical index or alias. Just update the java doc `@param index Index name or alias name`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org