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/10/24 07:23:00 UTC
[GitHub] [iotdb] LebronAl commented on a change in pull request #1854: [IOTDB-884] group createMultiTimeseriesPlan by partitionGroup
LebronAl commented on a change in pull request #1854:
URL: https://github.com/apache/iotdb/pull/1854#discussion_r511329759
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1478,6 +1479,11 @@ private TSStatus forwardPlan(Map<PhysicalPlan, PartitionGroup> planGroupMap, Phy
// belongs to NodeB, when NodeA returns a success while NodeB returns a failure, the
// failure and success should be placed into proper positions in TSStatus.subStatus
status = forwardInsertTabletPlan(planGroupMap, (InsertTabletPlan) plan);
+ } else if (plan instanceof CreateMultiTimeSeriesPlan) {
+ // CreateMultiTimeSeriesPlans contain many rows, each will correspond to a TSStatus as its
+ // execution result, as the plan is split and the sub-plans may have interleaving ranges,
+ // we must assure that each TSStatus is placed to the right position
Review comment:
Minor Suggesion:
```
// CreateMultiTimeSeriesPlan contains many rows, each will correspond to a TSStatus as its
// execution result, as the plan is splited and the sub-plans may have interleaving ranges,
// we must assure that each TSStatus is placed to the right position
```
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1505,6 +1511,48 @@ private TSStatus forwardPlan(Map<PhysicalPlan, PartitionGroup> planGroupMap, Phy
return status;
}
+ /**
+ * Forward each sub-plan to its belonging data group, and combine responses from the groups.
+ *
+ * @param planGroupMap sub-plan -> data group pairs
+ */
+ private TSStatus forwardCreateMultiTimeSeriesPlan(Map<PhysicalPlan, PartitionGroup> planGroupMap,
+ CreateMultiTimeSeriesPlan plan) {
+ List<String> errorCodePartitionGroups = new ArrayList<>();
+ TSStatus tmpStatus;
+ CreateMultiTimeSeriesPlan subPlan;
+ Map<Integer, Exception> results = new HashMap<>();
+ boolean noFailure = true;
+ boolean isBatchFailure = false;
+ // send sub-plans to each belonging data group and collect results
+ for (Map.Entry<PhysicalPlan, PartitionGroup> entry : planGroupMap.entrySet()) {
+ tmpStatus = forwardToSingleGroup(entry);
+ subPlan = (CreateMultiTimeSeriesPlan) entry.getKey();
+ logger.debug("{}: from {},{},{}", name, entry.getKey(), entry.getValue(), tmpStatus);
+ noFailure =
+ (tmpStatus.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode()) && noFailure;
+ isBatchFailure = (tmpStatus.getCode() == TSStatusCode.MULTIPLE_ERROR.getStatusCode())
Review comment:
When will `forwardToSingleGroup` return a TSStatusCode.MULTIPLE_ERROR during creating multi timeseries? It seems that this TSStatus will only be returned by BatchInsertionException, which is only thrown during 'insertTablet' interface currently~
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1505,6 +1511,48 @@ private TSStatus forwardPlan(Map<PhysicalPlan, PartitionGroup> planGroupMap, Phy
return status;
}
+ /**
+ * Forward each sub-plan to its belonging data group, and combine responses from the groups.
+ *
+ * @param planGroupMap sub-plan -> data group pairs
+ */
+ private TSStatus forwardCreateMultiTimeSeriesPlan(Map<PhysicalPlan, PartitionGroup> planGroupMap,
+ CreateMultiTimeSeriesPlan plan) {
+ List<String> errorCodePartitionGroups = new ArrayList<>();
+ TSStatus tmpStatus;
+ CreateMultiTimeSeriesPlan subPlan;
+ Map<Integer, Exception> results = new HashMap<>();
+ boolean noFailure = true;
+ boolean isBatchFailure = false;
+ // send sub-plans to each belonging data group and collect results
+ for (Map.Entry<PhysicalPlan, PartitionGroup> entry : planGroupMap.entrySet()) {
+ tmpStatus = forwardToSingleGroup(entry);
+ subPlan = (CreateMultiTimeSeriesPlan) entry.getKey();
+ logger.debug("{}: from {},{},{}", name, entry.getKey(), entry.getValue(), tmpStatus);
+ noFailure =
+ (tmpStatus.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode()) && noFailure;
+ isBatchFailure = (tmpStatus.getCode() == TSStatusCode.MULTIPLE_ERROR.getStatusCode())
+ || isBatchFailure;
+ if (tmpStatus.getCode() == TSStatusCode.MULTIPLE_ERROR.getStatusCode()) {
+ results.putAll(subPlan.getResults());
+ }
+ if (tmpStatus.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ // execution failed, record the error message
+ errorCodePartitionGroups.add(String.format("[%s@%s:%s:%s]",
+ tmpStatus.getCode(), entry.getValue().getHeader(),
+ tmpStatus.getMessage(), tmpStatus.subStatus));
+ }
+ }
+
+ plan.setResults(results);
+ if (noFailure || isBatchFailure) {
+ return RpcUtils.getStatus(TSStatusCode.SUCCESS_STATUS);
Review comment:
Why not use `StatusUtils.OK`? then we can reduce a creation of a TSStatus object with the same meaning.
----------------------------------------------------------------
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