You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2020/06/21 16:28:51 UTC

[GitHub] [incubator-iotdb] Alima777 opened a new pull request #1399: [IOTDB-736] Query performance tracing

Alima777 opened a new pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399


   This PR provides the performance tracing function by **adding a performance log** and gives a switch statement "**Tracing on/off**" to control whether to print this log, which is off by default.
   
   The log records including:
   1. the query sql, and the start time of this query
   3. the number of series paths
   4. the number of distinct tsfiles, including sequence and unsequence
   5. the number of chunks, and average size of each chunk.
   
   This pr concentrates on raw data query, it may modify something for aggregation query and group by query later.
   


----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r445594024



##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConstant.java
##########
@@ -97,6 +97,8 @@ private IoTDBConstant() {
   public static final String SCHEMA_FOLDER_NAME = "schema";
   public static final String SYNC_FOLDER_NAME = "sync";
   public static final String QUERY_FOLDER_NAME = "query";
+  public static final String TRACING_FOLDER_NAME = "tracing";

Review comment:
       It's used in IoTDBDescriptor `conf.setTracingDir(FilePathUtils.regularizePath(conf.getBaseDir() + IoTDBConstant.TRACING_FOLDER_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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r445597114



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writePathsNum(long queryId, int pathsNum) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum).toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeTsFileInfo(long queryId, int seqFileNum, int unseqFileNum) throws IOException {
+    // to avoid the disorder info of multi query
+    // add query id as prefix of each info
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of tsfiles: ").append(seqFileNum + unseqFileNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of sequence files: ").append(seqFileNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of unsequence files: ").append(unseqFileNum).toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeChunksInfo(long queryId, long totalChunkNum, long totalChunkSize) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of chunks: ").append(totalChunkNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Average size of chunks: ").append(totalChunkSize / totalChunkNum).toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeEndTime(long queryId) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - End time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  private static class TracingManagerHelper {
+
+    private static final TracingManager INSTANCE = new TracingManager(
+        IoTDBDescriptor.getInstance().getConfig().getTracingDir(),

Review comment:
       All above fixed.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] jixuan1989 merged pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
jixuan1989 merged pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399


   


----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r445594443



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);

Review comment:
       Fixed.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r446200347



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName) {
+    File tracingDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!tracingDir.exists()) {
+      if (tracingDir.mkdirs()) {
+        logger.info("create performance folder {}.", tracingDir);
+      } else {
+        logger.info("create performance folder {} failed.", tracingDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();
+  }
+
+  public void writePathsNum(long queryId, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();

Review comment:
       Actually, I researched and also did a test by myself, the process of BufferedWriter.write() self is thread safe...
   For example, thread A invokes `writer.write("aaa")`, thread B invokes `writer.write("bbb")`, the result will only be "aaabbb" Or "bbbaaa". So the `synchronized` is not needed. And writer.flush() is also needed.....
   
   Sorry, I will change all back. Remove `synchronized` and add flush() back.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r445592595



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +110,50 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      seqFileNumMap.computeIfAbsent(context.getQueryId(), k -> new HashSet<>())
+          .addAll(queryDataSource.getSeqResources());

Review comment:
       You can see the Code in QueryResourceManager 136-146.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r446180954



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +110,50 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      seqFileNumMap.computeIfAbsent(context.getQueryId(), k -> new HashSet<>())
+          .addAll(queryDataSource.getSeqResources());

Review comment:
       But if the first time series is not kept in this map, we can not calculate the total number of distinct tsfiles in the whole query. Or we give the accessed times of tsfile instead? In this way, we only keep a accessed number for one time series.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] jixuan1989 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r446148588



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName) {
+    File tracingDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!tracingDir.exists()) {
+      if (tracingDir.mkdirs()) {
+        logger.info("create performance folder {}.", tracingDir);
+      } else {
+        logger.info("create performance folder {} failed.", tracingDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();
+  }
+
+  public void writePathsNum(long queryId, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();

Review comment:
       considering thread A writes "abcdefg", and thread B writes  "\n", then the result may be "abc\ndefg".
   
   (By the way, why not add "\n" into the end of the string?)




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r446593235



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +110,50 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      seqFileNumMap.computeIfAbsent(context.getQueryId(), k -> new HashSet<>())
+          .addAll(queryDataSource.getSeqResources());

Review comment:
       And it will reflect the correct performance in case the tsfileResources are GC and reloaded from disk in the future.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] jixuan1989 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r446146178



##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfig.java
##########
@@ -185,6 +184,11 @@
    */
   private String syncDir = "data" + File.separator + "system" + File.separator + "sync";
 
+  /**
+   * Performance tracing directory, stores performance tracing files
+   */
+  private String tracingDir = "data" + File.separator + "tracing";

Review comment:
       here should be "TRACING_FOLDER_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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r446200347



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName) {
+    File tracingDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!tracingDir.exists()) {
+      if (tracingDir.mkdirs()) {
+        logger.info("create performance folder {}.", tracingDir);
+      } else {
+        logger.info("create performance folder {} failed.", tracingDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();
+  }
+
+  public void writePathsNum(long queryId, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();

Review comment:
       Actually, I researched and also did a test by myself, the process of BufferedWriter.write() self is thread safe...
   For example, thread A invokes `writer.write("aaa")`, thread B invokes `writer.write("bbb")`, the result will only be "aaabbb" Or "bbbaaa". So the `synchronized` is not needed. And writer.flush() is also needed.....
   
   Sorry, I will changed all back. Remove `synchronized` and add flush() back.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r443922053



##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -223,6 +226,20 @@ public void enterFlush(FlushContext ctx) {
     initializedOperator = flushOperator;
   }
 
+  @Override
+  public void enterTracingOn(TracingOnContext ctx) {
+    super.enterTracingOn(ctx);
+    IoTDBDescriptor.getInstance().getConfig().setEnablePerformanceTracing(true);

Review comment:
       Fixed.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r443535053



##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java
##########
@@ -125,24 +138,23 @@ private IoTDBConfigCheck() {
     systemProperties.put(PARTITION_INTERVAL_STRING, String.valueOf(partitionInterval));
     systemProperties.put(TSFILE_FILE_SYSTEM_STRING, tsfileFileSystem);
     systemProperties.put(ENABLE_PARTITION_STRING, String.valueOf(enablePartition));
+    systemProperties.put(ENABLE_PERFORMANCE_TRACING, String.valueOf(enablePerformanceTracing));
     systemProperties.put(TAG_ATTRIBUTE_SIZE_STRING, tagAttributeTotalSize);
     systemProperties.put(MAX_DEGREE_OF_INDEX_STRING, maxDegreeOfIndexNode);
   }
 
 
   /**
    * check configuration in system.properties when starting IoTDB
-   *
+   * <p>
    * When init: create system.properties directly
-   *
-   * When upgrading the system.properties:
-   * (1) create system.properties.tmp
-   * (2) delete system.properties
-   * (2) rename system.properties.tmp to system.properties
+   * <p>
+   * When upgrading the system.properties: (1) create system.properties.tmp (2) delete

Review comment:
       OK, my fault, all changes back..




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#issuecomment-647349562


   > Great work!
   > Just one suggestion. It would be better to add some doc of this new feature. :)
   
   Thank you~ I will add it later. :D


----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r445591547



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +110,50 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      seqFileNumMap.computeIfAbsent(context.getQueryId(), k -> new HashSet<>())
+          .addAll(queryDataSource.getSeqResources());

Review comment:
       Hi, the TsFileResources used for this query in this map will be removed after printing tracing log. Then it can be cleaned by GC.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] JulianFeinauer commented on pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
JulianFeinauer commented on pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#issuecomment-647165445


   Good work but I would suggest to write the log file directly not via logger. Because a user could disable it by accident although the property is active. 


----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r443539139



##########
File path: server/src/main/java/org/apache/iotdb/db/query/reader/series/SeriesReader.java
##########
@@ -251,6 +257,13 @@ private void unpackAllOverlappedTimeSeriesMetadataToCachedChunkMetadata(
       unpackOneTimeSeriesMetadata(firstTimeSeriesMetadata);
       firstTimeSeriesMetadata = null;
     }
+
+    // try to calculate the total number of chunk and time-value points in chunk
+    if (IoTDBDescriptor.getInstance().getConfig().isEnablePerformanceTracing()) {
+      totalChunkNum += cachedChunkMetadata.size();

Review comment:
       Good suggestion, Fixed.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r446593110



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +110,50 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      seqFileNumMap.computeIfAbsent(context.getQueryId(), k -> new HashSet<>())
+          .addAll(queryDataSource.getSeqResources());

Review comment:
       Hi, I replace those objects with WeakReference as you said. I believe it will make sense in the GC. Thank you very much for your good suggestion. Please check it.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] qiaojialin commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
qiaojialin commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r444039820



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +109,49 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      Set<TsFileResource> seqFileNum = seqFileNumMap.get(context.getQueryId());
+      Set<TsFileResource> unseqFileNum = unseqFileNumMap.get(context.getQueryId());

Review comment:
       you could use computeIfAbsent

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +109,49 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      Set<TsFileResource> seqFileNum = seqFileNumMap.get(context.getQueryId());
+      Set<TsFileResource> unseqFileNum = unseqFileNumMap.get(context.getQueryId());
+      if (seqFileNum == null) {
+        seqFileNumMap.put(context.getQueryId(), new HashSet<>(queryDataSource.getSeqResources()));
+      } else {
+        seqFileNum.addAll(queryDataSource.getSeqResources());
+      }
+      if (unseqFileNum == null) {
+        unseqFileNumMap.put(context.getQueryId(), new HashSet<>(queryDataSource.getUnseqResources()));
+      } else {
+        unseqFileNum.addAll(queryDataSource.getUnseqResources());
+      }
+    }
+    return queryDataSource;
   }
 
   /**
    * Whenever the jdbc request is closed normally or abnormally, this method must be invoked. All
    * query tokens created by this jdbc request must be cleared.
    */
   public void endQuery(long queryId) throws StorageEngineException {
+    try {
+      if (config.isEnablePerformanceTracing() && chunkNumMap.get(queryId) != null) {

Review comment:
       Not all query unpacks the TimeseriesMetadata, we only unpack it when needed. So the chunkNumMap may do not contain the queryId, you'd better check the seqFileNum or check one by one

##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfig.java
##########
@@ -185,6 +184,11 @@
    */
   private String syncDir = "data" + File.separator + "system" + File.separator + "sync";
 
+  /**
+   * Performance tracing directory, stores performance tracing files
+   */
+  private String performanceDir = "data" + File.separator + "performance";

Review comment:
       ```suggestion
     private String performanceDir = "data" + File.separator + "tracing";
   ```

##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConstant.java
##########
@@ -97,6 +97,8 @@ private IoTDBConstant() {
   public static final String SCHEMA_FOLDER_NAME = "schema";
   public static final String SYNC_FOLDER_NAME = "sync";
   public static final String QUERY_FOLDER_NAME = "query";
+  public static final String PERFORMANCE_FOLDER_NAME = "performance";

Review comment:
       ```suggestion
     public static final String TRACING_FOLDER_NAME = "tracing";
   ```

##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConstant.java
##########
@@ -97,6 +97,8 @@ private IoTDBConstant() {
   public static final String SCHEMA_FOLDER_NAME = "schema";
   public static final String SYNC_FOLDER_NAME = "sync";
   public static final String QUERY_FOLDER_NAME = "query";
+  public static final String PERFORMANCE_FOLDER_NAME = "performance";
+  public static final String PERFORMANCE_LOG = "performance.txt";

Review comment:
       ```suggestion
     public static final String PERFORMANCE_LOG = "tracing.txt";
   ```

##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -548,6 +568,11 @@ private TSExecuteStatementResp internalExecuteQueryStatement(String statement,
       resp.setOperationType(plan.getOperatorType().toString());
       // generate the queryId for the operation
       queryId = generateQueryId(true);
+      if (plan instanceof QueryPlan && config.isEnablePerformanceTracing()) {
+        TracingManager.getInstance().writeQueryId(queryId);
+        TracingManager.getInstance().writeStartTime();
+        TracingManager.getInstance().writePathsNum(plan.getPaths().size());

Review comment:
       put it to one method, this method write one line and \n

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeSeperator() throws IOException {
+    writer.write("-----------------------------");
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeStatement(String statement) throws IOException {

Review comment:
       each line should have a QueryId in prefix

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -47,6 +55,14 @@
 
   private AtomicLong queryIdAtom = new AtomicLong();
   private QueryFileManager filePathsManager;
+  private static final Logger logger = LoggerFactory.getLogger(QueryResourceManager.class);
+  // record the total number and size of chunks for each query id
+  private Map<Long, Long> chunkNumMap = new ConcurrentHashMap<>();
+  private Map<Long, Long> chunkSizeMap = new ConcurrentHashMap<>();

Review comment:
       add javadoc, the chunk size is the number of points in chunk




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r444073451



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeSeperator() throws IOException {
+    writer.write("-----------------------------");
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeStatement(String statement) throws IOException {

Review comment:
       All above Fixed. Thank you for detailed review :)




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r443534809



##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfig.java
##########
@@ -185,6 +184,11 @@
    */
   private String syncDir = "data" + File.separator + "system" + File.separator + "sync";
 
+  /**
+   * Performance tracing directory, stores performance tracing files
+   */
+  private String performanceDir = "data" + File.separator + "system" + File.separator + "performance";

Review comment:
       Fixed.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r444020388



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
##########
@@ -1233,6 +1237,20 @@ public QueryDataSource query(String deviceId, String measurementId, QueryContext
       if (filePathsManager != null) {
         filePathsManager.addUsedFilesForQuery(context.getQueryId(), dataSource);
       }
+
+      // exclude repetitive tsFile to calculate the number of it
+      if (config.isEnablePerformanceTracing()) {
+        if (seqFile == null) {
+          seqFile = new HashSet<>(seqResources);

Review comment:
       Fixed.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] jixuan1989 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r446147890



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName) {
+    File tracingDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!tracingDir.exists()) {
+      if (tracingDir.mkdirs()) {
+        logger.info("create performance folder {}.", tracingDir);
+      } else {
+        logger.info("create performance folder {} failed.", tracingDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();
+  }
+
+  public void writePathsNum(long queryId, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    synchronized (writer) {
+      writer.write(builder.toString());
+    }
+    writer.newLine();

Review comment:
       `writer.newLine()`  also needs to be wrapped... 
   




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r443637619



##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -524,10 +546,30 @@ public TSExecuteStatementResp executeQueryStatement(TSExecuteStatementReq req) {
    *             AuthorPlan
    */
   private TSExecuteStatementResp internalExecuteQueryStatement(String statement,
-      long statementId, PhysicalPlan plan, int fetchSize, String username) {
+      long statementId, PhysicalPlan plan, int fetchSize, String username) throws IOException {
     auditLogger.info("Session {} execute Query: {}", currSessionId.get(), statement);
     long startTime = System.currentTimeMillis();
     long queryId = -1;
+    if (plan instanceof QueryPlan && config.isEnablePerformanceTracing()) {
+      File performanceDir = SystemFileFactory.INSTANCE.getFile(config.getPerformanceDir());

Review comment:
       Fixed.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] qiaojialin commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
qiaojialin commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r444086465



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +110,53 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      Set<TsFileResource> seqFileNum = seqFileNumMap.get(context.getQueryId());
+      Set<TsFileResource> unseqFileNum = unseqFileNumMap.get(context.getQueryId());
+      if (seqFileNum == null) {
+        seqFileNumMap.put(context.getQueryId(), new HashSet<>(queryDataSource.getSeqResources()));
+      } else {
+        seqFileNum.addAll(queryDataSource.getSeqResources());
+      }
+      if (unseqFileNum == null) {
+        unseqFileNumMap.put(context.getQueryId(), new HashSet<>(queryDataSource.getUnseqResources()));
+      } else {
+        unseqFileNum.addAll(queryDataSource.getUnseqResources());
+      }
+    }
+    return queryDataSource;
   }
 
   /**
    * Whenever the jdbc request is closed normally or abnormally, this method must be invoked. All
    * query tokens created by this jdbc request must be cleared.
    */
   public void endQuery(long queryId) throws StorageEngineException {
+    try {
+      if (config.isEnablePerformanceTracing()) {
+        if (seqFileNumMap.get(queryId) != null && unseqFileNumMap.get(queryId) != null) {
+          TracingManager.getInstance().writeTsFileInfo(queryId, seqFileNumMap.remove(queryId).size(),
+              unseqFileNumMap.remove(queryId).size());
+        }
+        if (chunkNumMap.get(queryId) != null && chunkSizeMap.get(queryId) != null) {
+          TracingManager.getInstance()
+              .writeChunksInfo(queryId, chunkNumMap.remove(queryId), chunkSizeMap.remove(queryId));
+        }
+      }
+    } catch (IOException e) {
+      logger.error(
+          "Error while writing performance info to {}, {}",
+          config.getTracingDir() + File.separator + IoTDBConstant.TRACING_LOG, e);

Review comment:
       ```suggestion
             config.getTracingDir() + File.separator + IoTDBConstant.TRACING_LOG, e.getMessage());
   
   ```




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] qiaojialin commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
qiaojialin commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r443442334



##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -223,6 +226,20 @@ public void enterFlush(FlushContext ctx) {
     initializedOperator = flushOperator;
   }
 
+  @Override
+  public void enterTracingOn(TracingOnContext ctx) {
+    super.enterTracingOn(ctx);
+    IoTDBDescriptor.getInstance().getConfig().setEnablePerformanceTracing(true);

Review comment:
       I suggest adding a parameter boolean in TracingOperator and TracingPlan, then set the config in PlanExecutor.

##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -524,10 +546,30 @@ public TSExecuteStatementResp executeQueryStatement(TSExecuteStatementReq req) {
    *             AuthorPlan
    */
   private TSExecuteStatementResp internalExecuteQueryStatement(String statement,
-      long statementId, PhysicalPlan plan, int fetchSize, String username) {
+      long statementId, PhysicalPlan plan, int fetchSize, String username) throws IOException {
     auditLogger.info("Session {} execute Query: {}", currSessionId.get(), statement);
     long startTime = System.currentTimeMillis();
     long queryId = -1;
+    if (plan instanceof QueryPlan && config.isEnablePerformanceTracing()) {
+      File performanceDir = SystemFileFactory.INSTANCE.getFile(config.getPerformanceDir());

Review comment:
       put all these performance related to a class TracingManager, which manage a writer, log file.

##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfig.java
##########
@@ -185,6 +184,11 @@
    */
   private String syncDir = "data" + File.separator + "system" + File.separator + "sync";
 
+  /**
+   * Performance tracing directory, stores performance tracing files
+   */
+  private String performanceDir = "data" + File.separator + "system" + File.separator + "performance";

Review comment:
       put this to logs folder or data/performance is better

##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java
##########
@@ -125,24 +138,23 @@ private IoTDBConfigCheck() {
     systemProperties.put(PARTITION_INTERVAL_STRING, String.valueOf(partitionInterval));
     systemProperties.put(TSFILE_FILE_SYSTEM_STRING, tsfileFileSystem);
     systemProperties.put(ENABLE_PARTITION_STRING, String.valueOf(enablePartition));
+    systemProperties.put(ENABLE_PERFORMANCE_TRACING, String.valueOf(enablePerformanceTracing));
     systemProperties.put(TAG_ATTRIBUTE_SIZE_STRING, tagAttributeTotalSize);
     systemProperties.put(MAX_DEGREE_OF_INDEX_STRING, maxDegreeOfIndexNode);
   }
 
 
   /**
    * check configuration in system.properties when starting IoTDB
-   *
+   * <p>
    * When init: create system.properties directly
-   *
-   * When upgrading the system.properties:
-   * (1) create system.properties.tmp
-   * (2) delete system.properties
-   * (2) rename system.properties.tmp to system.properties
+   * <p>
+   * When upgrading the system.properties: (1) create system.properties.tmp (2) delete

Review comment:
       format all changes of this class back..

##########
File path: server/src/main/java/org/apache/iotdb/db/query/reader/series/SeriesReader.java
##########
@@ -251,6 +257,13 @@ private void unpackAllOverlappedTimeSeriesMetadataToCachedChunkMetadata(
       unpackOneTimeSeriesMetadata(firstTimeSeriesMetadata);
       firstTimeSeriesMetadata = null;
     }
+
+    // try to calculate the total number of chunk and time-value points in chunk
+    if (IoTDBDescriptor.getInstance().getConfig().isEnablePerformanceTracing()) {
+      totalChunkNum += cachedChunkMetadata.size();

Review comment:
       This method may be called many times before the cachedChunkMetadata is all consumed. You can put this tracing in unpackOneTimeSeriesMetadata, i.e., when we call FileLoaderUtils.

##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
##########
@@ -1233,6 +1237,20 @@ public QueryDataSource query(String deviceId, String measurementId, QueryContext
       if (filePathsManager != null) {
         filePathsManager.addUsedFilesForQuery(context.getQueryId(), dataSource);
       }
+
+      // exclude repetitive tsFile to calculate the number of it
+      if (config.isEnablePerformanceTracing()) {
+        if (seqFile == null) {
+          seqFile = new HashSet<>(seqResources);

Review comment:
       If I query multiple times, when do you clear this?

##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java
##########
@@ -80,11 +87,16 @@
 
   private static final String ERROR_LOG = "Wrong %s, please set as: %s !";
 
+  private static final String ENABLE_PERFORMANCE_TRACING = "enable_performance_tracing";

Review comment:
       this is not an invariant parameter, no need to put it here

##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java
##########
@@ -125,24 +138,23 @@ private IoTDBConfigCheck() {
     systemProperties.put(PARTITION_INTERVAL_STRING, String.valueOf(partitionInterval));
     systemProperties.put(TSFILE_FILE_SYSTEM_STRING, tsfileFileSystem);
     systemProperties.put(ENABLE_PARTITION_STRING, String.valueOf(enablePartition));
+    systemProperties.put(ENABLE_PERFORMANCE_TRACING, String.valueOf(enablePerformanceTracing));

Review comment:
       do not modify this class

##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -223,6 +226,20 @@ public void enterFlush(FlushContext ctx) {
     initializedOperator = flushOperator;
   }
 
+  @Override
+  public void enterTracingOn(TracingOnContext ctx) {
+    super.enterTracingOn(ctx);
+    IoTDBDescriptor.getInstance().getConfig().setEnablePerformanceTracing(true);

Review comment:
       I suggest adding a parameter boolean in TracingOperator and TracingPlan, then set the config in PlanExecutor.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] HTHou commented on pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
HTHou commented on pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#issuecomment-647348966


   Great work!
   Just one suggestion. It would be better to add some doc of this new feature. :)


----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r445600307



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +109,49 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      Set<TsFileResource> seqFileNum = seqFileNumMap.get(context.getQueryId());
+      Set<TsFileResource> unseqFileNum = unseqFileNumMap.get(context.getQueryId());
+      if (seqFileNum == null) {
+        seqFileNumMap.put(context.getQueryId(), new HashSet<>(queryDataSource.getSeqResources()));
+      } else {
+        seqFileNum.addAll(queryDataSource.getSeqResources());
+      }
+      if (unseqFileNum == null) {
+        unseqFileNumMap.put(context.getQueryId(), new HashSet<>(queryDataSource.getUnseqResources()));
+      } else {
+        unseqFileNum.addAll(queryDataSource.getUnseqResources());
+      }
+    }
+    return queryDataSource;
   }
 
   /**
    * Whenever the jdbc request is closed normally or abnormally, this method must be invoked. All
    * query tokens created by this jdbc request must be cleared.
    */
   public void endQuery(long queryId) throws StorageEngineException {
+    try {
+      if (config.isEnablePerformanceTracing() && chunkNumMap.get(queryId) != null) {

Review comment:
       Fixed.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#issuecomment-647238675


   > Good work but I would suggest to write the log file directly not via logger. Because a user could disable it by accident although the property is active.
   
   Thank you for your good suggestion. I've fixed it.


----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] jixuan1989 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r445399603



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writePathsNum(long queryId, int pathsNum) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum).toString());
+    writer.newLine();
+    writer.flush();

Review comment:
       use StringBuilder to get string first (do not need to wrap with `sync`), and wrap `writer.write` with `sync`

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();

Review comment:
       same issue.

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writePathsNum(long queryId, int pathsNum) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum).toString());
+    writer.newLine();
+    writer.flush();

Review comment:
       same issue

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writePathsNum(long queryId, int pathsNum) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum).toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeTsFileInfo(long queryId, int seqFileNum, int unseqFileNum) throws IOException {
+    // to avoid the disorder info of multi query
+    // add query id as prefix of each info
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of tsfiles: ").append(seqFileNum + unseqFileNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of sequence files: ").append(seqFileNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of unsequence files: ").append(unseqFileNum).toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeChunksInfo(long queryId, long totalChunkNum, long totalChunkSize) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of chunks: ").append(totalChunkNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Average size of chunks: ").append(totalChunkSize / totalChunkNum).toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeEndTime(long queryId) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - End time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  private static class TracingManagerHelper {
+
+    private static final TracingManager INSTANCE = new TracingManager(
+        IoTDBDescriptor.getInstance().getConfig().getTracingDir(),

Review comment:
       same issue

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +110,50 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      seqFileNumMap.computeIfAbsent(context.getQueryId(), k -> new HashSet<>())
+          .addAll(queryDataSource.getSeqResources());

Review comment:
       Though currently we put all TsResources into the memory, but we may change this design in the future. 
   However, you put the TsResources into the map here, which will lead to the GC failed for those objects.
   
   How about considering Weak/PhantomReference references  (not sure whether it is ok. https://juejin.im/post/5b82c02df265da436152f5ad)

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    writer.write(builder.toString());

Review comment:
       `writer.write()` should be protected by `synchnorized`

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();

Review comment:
       no need to call `flush`

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);

Review comment:
       perfromanceDir -> tracingDir

##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConstant.java
##########
@@ -97,6 +97,8 @@ private IoTDBConstant() {
   public static final String SCHEMA_FOLDER_NAME = "schema";
   public static final String SYNC_FOLDER_NAME = "sync";
   public static final String QUERY_FOLDER_NAME = "query";
+  public static final String TRACING_FOLDER_NAME = "tracing";

Review comment:
       not used

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writePathsNum(long queryId, int pathsNum) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum).toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeTsFileInfo(long queryId, int seqFileNum, int unseqFileNum) throws IOException {
+    // to avoid the disorder info of multi query
+    // add query id as prefix of each info
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of tsfiles: ").append(seqFileNum + unseqFileNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of sequence files: ").append(seqFileNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of unsequence files: ").append(unseqFileNum).toString());
+    writer.newLine();
+    writer.flush();

Review comment:
       same issue

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/TracingManager.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iotdb.db.query.control;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.text.SimpleDateFormat;
+import org.apache.iotdb.db.conf.IoTDBConstant;
+import org.apache.iotdb.db.conf.IoTDBDescriptor;
+import org.apache.iotdb.db.engine.fileSystem.SystemFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TracingManager {
+
+  private static final Logger logger = LoggerFactory.getLogger(TracingManager.class);
+  private BufferedWriter writer;
+
+  public TracingManager(String dirName, String logFileName){
+    File performanceDir = SystemFileFactory.INSTANCE.getFile(dirName);
+    if (!performanceDir.exists()) {
+      if (performanceDir.mkdirs()) {
+        logger.info("create performance folder {}.", performanceDir);
+      } else {
+        logger.info("create performance folder {} failed.", performanceDir);
+      }
+    }
+    File logFile = SystemFileFactory.INSTANCE.getFile(dirName + File.separator + logFileName);
+
+    FileWriter fileWriter = null;
+    try {
+      fileWriter = new FileWriter(logFile, true);
+    } catch (IOException e) {
+      logger.error("Meeting error while creating TracingManager: {}", e);
+    }
+    writer = new BufferedWriter(fileWriter);
+  }
+
+  public static TracingManager getInstance() {
+    return TracingManagerHelper.INSTANCE;
+  }
+
+  public void writeQueryInfo(long queryId, String statement, int pathsNum) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()))
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum);
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  // for align by device query
+  public void writeQueryInfo(long queryId, String statement) throws IOException {
+    StringBuilder builder = new StringBuilder("-----------------------------\n");
+    builder.append("Query Id: ").append(queryId)
+        .append(" - Query Statement: ").append(statement)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Start time: ")
+        .append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(System.currentTimeMillis()));
+    writer.write(builder.toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writePathsNum(long queryId, int pathsNum) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of series paths: ").append(pathsNum).toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeTsFileInfo(long queryId, int seqFileNum, int unseqFileNum) throws IOException {
+    // to avoid the disorder info of multi query
+    // add query id as prefix of each info
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of tsfiles: ").append(seqFileNum + unseqFileNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of sequence files: ").append(seqFileNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Number of unsequence files: ").append(unseqFileNum).toString());
+    writer.newLine();
+    writer.flush();
+  }
+
+  public void writeChunksInfo(long queryId, long totalChunkNum, long totalChunkSize) throws IOException {
+    writer.write(new StringBuilder("Query Id: ").append(queryId)
+        .append(" - Number of chunks: ").append(totalChunkNum)
+        .append("\nQuery Id: ").append(queryId)
+        .append(" - Average size of chunks: ").append(totalChunkSize / totalChunkNum).toString());
+    writer.newLine();
+    writer.flush();

Review comment:
       same issue




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] jixuan1989 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r446147306



##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +110,50 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      seqFileNumMap.computeIfAbsent(context.getQueryId(), k -> new HashSet<>())
+          .addAll(queryDataSource.getSeqResources());

Review comment:
       this method just query one time series.. 
   Suppose we have 2 time series to be queried, maybe the resources corresponding to the first time series can be GC released before the second one is queried.

##########
File path: server/src/main/java/org/apache/iotdb/db/query/control/QueryResourceManager.java
##########
@@ -85,20 +110,50 @@ public void registerTempExternalSortFile(long queryId,
     externalSortFileMap.computeIfAbsent(queryId, x -> new ArrayList<>()).add(deserializer);
   }
 
-
   public QueryDataSource getQueryDataSource(Path selectedPath,
       QueryContext context, Filter filter) throws StorageEngineException, QueryProcessException {
 
     SingleSeriesExpression singleSeriesExpression = new SingleSeriesExpression(selectedPath,
         filter);
-    return StorageEngine.getInstance().query(singleSeriesExpression, context, filePathsManager);
+    QueryDataSource queryDataSource = StorageEngine.getInstance()
+        .query(singleSeriesExpression, context, filePathsManager);
+    // calculate the distinct number of seq and unseq tsfiles
+    if (config.isEnablePerformanceTracing()) {
+      seqFileNumMap.computeIfAbsent(context.getQueryId(), k -> new HashSet<>())
+          .addAll(queryDataSource.getSeqResources());

Review comment:
       this method just queries one time series.. 
   Suppose we have 2 time series to be queried, maybe the resources corresponding to the first time series can be GC released before the second one is queried.




----------------------------------------------------------------
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.

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



[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1399: [IOTDB-736] Query performance tracing

Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1399:
URL: https://github.com/apache/incubator-iotdb/pull/1399#discussion_r446183993



##########
File path: server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfig.java
##########
@@ -185,6 +184,11 @@
    */
   private String syncDir = "data" + File.separator + "system" + File.separator + "sync";
 
+  /**
+   * Performance tracing directory, stores performance tracing files
+   */
+  private String tracingDir = "data" + File.separator + "tracing";

Review comment:
       I think you are right. But the Dir strings in this file are all defined this way....




----------------------------------------------------------------
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.

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