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 2022/02/17 05:53:35 UTC

[GitHub] [iotdb] ericpai opened a new pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

ericpai opened a new pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072


   See JIRA https://issues.apache.org/jira/browse/IOTDB-2557


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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] ericpai commented on pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
ericpai commented on pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#issuecomment-1044084106


   Can someone help to review these codes?


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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] OneSizeFitsQuorum commented on pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum commented on pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#issuecomment-1045709643


   @Alima777  CC


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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] ericpai commented on a change in pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#discussion_r810613736



##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/query/LocalQueryExecutor.java
##########
@@ -602,16 +602,15 @@ public long querySingleSeriesByTimestamp(SingleSeriesQueryRequest request)
     }
   }
 
-  public ByteBuffer getAllMeasurementSchema(ByteBuffer planBuffer)
+  public ByteBuffer getAllMeasurementSchema(MeasurementSchemaRequest request)
       throws CheckConsistencyException, IOException, MetadataException {
     dataGroupMember.syncLeaderWithConsistencyCheck(false);
 
-    ShowTimeSeriesPlan plan = (ShowTimeSeriesPlan) PhysicalPlan.Factory.create(planBuffer);
+    ShowTimeSeriesPlan plan = (ShowTimeSeriesPlan) PhysicalPlan.Factory.create(request.planBinary);
     List<ShowTimeSeriesResult> allTimeseriesSchema;
-    allTimeseriesSchema =
-        getCMManager()
-            .showLocalTimeseries(
-                plan, new QueryContext(SessionManager.getInstance().requestQueryId(false)));
+    RemoteQueryContext queryContext =
+        queryManager.getQueryContext(request.getRequester(), request.getQueryId());
+    allTimeseriesSchema = getCMManager().showLocalTimeseries(plan, queryContext);

Review comment:
       @OneSizeFitsQuorum I debug this case and see that the queryId is assigned at QueryTask.call(). (Here the queryId is 31)
   ![image](https://user-images.githubusercontent.com/3821212/154840951-c5510949-3bbf-4fc6-8abd-429166d63b5f.png)
   
   And the query resource is released in `FetchResultTask` and `SessionManager.closeDataSet()` (The query id is 31 as well)
   
   ![image](https://user-images.githubusercontent.com/3821212/154841036-2b5c9a9a-fa1e-4ab4-b314-e5bc6499f9cf.png)
   ![image](https://user-images.githubusercontent.com/3821212/154841094-e742ee0b-d3a8-4a5b-a8be-837d846e3c7a.png)
   
   For any query, it will execute the query firstly and get a query id. Then it will fetch results, if necessary. Fetching results request will be sent with this query id. When fetching results ends, it will release all local and remote query resources.
   
   So I think there's no resource leak. And you can reference `ClusterSessionManager.releaseQueryResource()` to get the logics of remote query resource release.
   




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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] ericpai commented on a change in pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#discussion_r810744466



##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -1603,6 +1608,9 @@ private void showRemoteTimeseries(
       } catch (InterruptedException e) {
         logger.error("Interrupted when getting timeseries schemas in node {}.", node, e);
         Thread.currentThread().interrupt();
+      } finally {
+        // record the queried node to release resources later
+        ((RemoteQueryContext) context).registerRemoteNode(node, group.getHeader());

Review comment:
       @OneSizeFitsQuorum Here I reference the `ClusterReaderFactory.getRemoteGroupByExecutor()` to handle the `registerRemoteNode` to make sure the remote resource can be actually released after query.




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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] coveralls edited a comment on pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#issuecomment-1042627957


   
   [![Coverage Status](https://coveralls.io/builds/46711006/badge)](https://coveralls.io/builds/46711006)
   
   Coverage decreased (-0.01%) to 67.754% when pulling **624d4cfbf37366b4544d14216de4d4f53db16ff3 on ericpai:bugfix/iotdb-2557** into **ba0c6ee014f3d07771f63fa1c95d64b515993f56 on apache:master**.
   


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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] ericpai merged pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
ericpai merged pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072


   


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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] coveralls commented on pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#issuecomment-1042627957


   
   [![Coverage Status](https://coveralls.io/builds/46629285/badge)](https://coveralls.io/builds/46629285)
   
   Coverage decreased (-0.03%) to 67.718% when pulling **45b405f34621df5f2e410c6dba65273629baca2f on ericpai:bugfix/iotdb-2557** into **9c332cfe528d7bfc62aaaa1ca1ed277950ef8b48 on apache:master**.
   


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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] ericpai commented on a change in pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#discussion_r810743979



##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/query/LocalQueryExecutor.java
##########
@@ -602,16 +602,15 @@ public long querySingleSeriesByTimestamp(SingleSeriesQueryRequest request)
     }
   }
 
-  public ByteBuffer getAllMeasurementSchema(ByteBuffer planBuffer)
+  public ByteBuffer getAllMeasurementSchema(MeasurementSchemaRequest request)
       throws CheckConsistencyException, IOException, MetadataException {
     dataGroupMember.syncLeaderWithConsistencyCheck(false);
 
-    ShowTimeSeriesPlan plan = (ShowTimeSeriesPlan) PhysicalPlan.Factory.create(planBuffer);
+    ShowTimeSeriesPlan plan = (ShowTimeSeriesPlan) PhysicalPlan.Factory.create(request.planBinary);
     List<ShowTimeSeriesResult> allTimeseriesSchema;
-    allTimeseriesSchema =
-        getCMManager()
-            .showLocalTimeseries(
-                plan, new QueryContext(SessionManager.getInstance().requestQueryId(false)));
+    RemoteQueryContext queryContext =
+        queryManager.getQueryContext(request.getRequester(), request.getQueryId());
+    allTimeseriesSchema = getCMManager().showLocalTimeseries(plan, queryContext);

Review comment:
       @OneSizeFitsQuorum Nice catch! It's my mistake to miss the `registerRemoteNode()`. I add this in a finally block in `private void showRemoteTimeseries(PartitionGroup group, ShowTimeSeriesPlan plan, QueryContext context, ShowTimeSeriesHandler handler)` in `CMManager`.




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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] coveralls edited a comment on pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#issuecomment-1042627957


   
   [![Coverage Status](https://coveralls.io/builds/46718698/badge)](https://coveralls.io/builds/46718698)
   
   Coverage increased (+0.05%) to 67.813% when pulling **d2eae039388650ca898208a7c9e43a9d8f48b62b on ericpai:bugfix/iotdb-2557** into **ba0c6ee014f3d07771f63fa1c95d64b515993f56 on apache:master**.
   


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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] OneSizeFitsQuorum commented on a change in pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum commented on a change in pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#discussion_r810656262



##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/query/LocalQueryExecutor.java
##########
@@ -602,16 +602,15 @@ public long querySingleSeriesByTimestamp(SingleSeriesQueryRequest request)
     }
   }
 
-  public ByteBuffer getAllMeasurementSchema(ByteBuffer planBuffer)
+  public ByteBuffer getAllMeasurementSchema(MeasurementSchemaRequest request)
       throws CheckConsistencyException, IOException, MetadataException {
     dataGroupMember.syncLeaderWithConsistencyCheck(false);
 
-    ShowTimeSeriesPlan plan = (ShowTimeSeriesPlan) PhysicalPlan.Factory.create(planBuffer);
+    ShowTimeSeriesPlan plan = (ShowTimeSeriesPlan) PhysicalPlan.Factory.create(request.planBinary);
     List<ShowTimeSeriesResult> allTimeseriesSchema;
-    allTimeseriesSchema =
-        getCMManager()
-            .showLocalTimeseries(
-                plan, new QueryContext(SessionManager.getInstance().requestQueryId(false)));
+    RemoteQueryContext queryContext =
+        queryManager.getQueryContext(request.getRequester(), request.getQueryId());
+    allTimeseriesSchema = getCMManager().showLocalTimeseries(plan, queryContext);

Review comment:
       `ClusterSessionManager.releaseQueryResource()` will release resource in the remote node only when the remote node is registered by `RemoteQueryContext.registerRemoteNode(Node node, RaftNode header)` during execution. 
   
   Now it's possible that showTimeseriesPlan holds the state of the remote node, but it doesn't seem to register the remote node during execution?
   
   <img width="1049" alt="image" src="https://user-images.githubusercontent.com/32640567/154854773-c6a82275-3e2d-4452-bce7-df5c24f07f43.png">
   




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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] OneSizeFitsQuorum commented on a change in pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum commented on a change in pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#discussion_r810656262



##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/query/LocalQueryExecutor.java
##########
@@ -602,16 +602,15 @@ public long querySingleSeriesByTimestamp(SingleSeriesQueryRequest request)
     }
   }
 
-  public ByteBuffer getAllMeasurementSchema(ByteBuffer planBuffer)
+  public ByteBuffer getAllMeasurementSchema(MeasurementSchemaRequest request)
       throws CheckConsistencyException, IOException, MetadataException {
     dataGroupMember.syncLeaderWithConsistencyCheck(false);
 
-    ShowTimeSeriesPlan plan = (ShowTimeSeriesPlan) PhysicalPlan.Factory.create(planBuffer);
+    ShowTimeSeriesPlan plan = (ShowTimeSeriesPlan) PhysicalPlan.Factory.create(request.planBinary);
     List<ShowTimeSeriesResult> allTimeseriesSchema;
-    allTimeseriesSchema =
-        getCMManager()
-            .showLocalTimeseries(
-                plan, new QueryContext(SessionManager.getInstance().requestQueryId(false)));
+    RemoteQueryContext queryContext =
+        queryManager.getQueryContext(request.getRequester(), request.getQueryId());
+    allTimeseriesSchema = getCMManager().showLocalTimeseries(plan, queryContext);

Review comment:
       `ClusterSessionManager.releaseQueryResource()` will release resource in the remote node only when the remote node is registered by `RemoteQueryContext.registerRemoteNode(Node node, RaftNode header)`. 
   
   Now it's possible that showTimeseriesPlan holds the state of the remote node, but it doesn't seem to register the remote node during execution?
   
   <img width="1049" alt="image" src="https://user-images.githubusercontent.com/32640567/154854773-c6a82275-3e2d-4452-bce7-df5c24f07f43.png">
   




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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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

[GitHub] [iotdb] OneSizeFitsQuorum commented on a change in pull request #5072: [IOTDB-2557] Fix non-data-read QueryContext in getAllMeasurementSchema

Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum commented on a change in pull request #5072:
URL: https://github.com/apache/iotdb/pull/5072#discussion_r810560727



##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/query/LocalQueryExecutor.java
##########
@@ -602,16 +602,15 @@ public long querySingleSeriesByTimestamp(SingleSeriesQueryRequest request)
     }
   }
 
-  public ByteBuffer getAllMeasurementSchema(ByteBuffer planBuffer)
+  public ByteBuffer getAllMeasurementSchema(MeasurementSchemaRequest request)
       throws CheckConsistencyException, IOException, MetadataException {
     dataGroupMember.syncLeaderWithConsistencyCheck(false);
 
-    ShowTimeSeriesPlan plan = (ShowTimeSeriesPlan) PhysicalPlan.Factory.create(planBuffer);
+    ShowTimeSeriesPlan plan = (ShowTimeSeriesPlan) PhysicalPlan.Factory.create(request.planBinary);
     List<ShowTimeSeriesResult> allTimeseriesSchema;
-    allTimeseriesSchema =
-        getCMManager()
-            .showLocalTimeseries(
-                plan, new QueryContext(SessionManager.getInstance().requestQueryId(false)));
+    RemoteQueryContext queryContext =
+        queryManager.getQueryContext(request.getRequester(), request.getQueryId());
+    allTimeseriesSchema = getCMManager().showLocalTimeseries(plan, queryContext);

Review comment:
       Looks reasonable, but if we pass the queryId this way, we need to send an endQuery request from the coordinator node after the query completes to clean up the resources in the queryManager, otherwise there will be a memory leak. So far I haven't found any code related to resource cleaning, what do you think?




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

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

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