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/27 01:54:17 UTC
[GitHub] [iotdb] jt2594838 commented on a change in pull request #1854: [IOTDB-884] group createMultiTimeseriesPlan by partitionGroup
jt2594838 commented on a change in pull request #1854:
URL: https://github.com/apache/iotdb/pull/1854#discussion_r512365107
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/query/ClusterPlanRouter.java
##########
@@ -292,4 +295,64 @@ private PartitionGroup routePlan(ShowChildPathsPlan plan) {
}
return result;
}
+
+ @SuppressWarnings("SuspiciousSystemArraycopy")
+ private Map<PhysicalPlan, PartitionGroup> splitAndRoutePlan(CreateMultiTimeSeriesPlan plan)
+ throws MetadataException {
+ Map<PhysicalPlan, PartitionGroup> result = new HashMap<>();
+ Map<PartitionGroup, PhysicalPlan> groupHoldPlan = new HashMap<>();
+
+ for (int i = 0; i < plan.getPaths().size(); i++) {
+ PartialPath path = plan.getPaths().get(i);
+ PartitionGroup partitionGroup =
+ partitionTable.partitionByPathTime(path, 0);
+ CreateMultiTimeSeriesPlan subPlan = null;
+ if (groupHoldPlan.get(partitionGroup) == null) {
Review comment:
Assign `groupHoldPlan.get(partitionGroup)` to a local variable so that you will not need to call `get` twice.
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1510,46 +1511,66 @@ private TSStatus forwardPlan(Map<PhysicalPlan, PartitionGroup> planGroupMap, Phy
*
* @param planGroupMap sub-plan -> data group pairs
*/
- private TSStatus forwardInsertTabletPlan(Map<PhysicalPlan, PartitionGroup> planGroupMap,
- InsertTabletPlan plan) {
+ private TSStatus forwardMultiSubPlan(Map<PhysicalPlan, PartitionGroup> planGroupMap,
+ PhysicalPlan plan) {
Review comment:
Maybe we should rename `plan` to something like `completePlan`, `originalPlan` or `parentPlan`.
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1466,18 +1467,18 @@ public TSStatus processPartitionedPlan(PhysicalPlan plan) throws UnsupportedPlan
*/
private TSStatus forwardPlan(Map<PhysicalPlan, PartitionGroup> planGroupMap, PhysicalPlan plan) {
// the error codes from the groups that cannot execute the plan
- TSStatus status;
+ TSStatus status = null;
if (planGroupMap.size() == 1) {
status = forwardToSingleGroup(planGroupMap.entrySet().iterator().next());
} else {
- if (plan instanceof InsertTabletPlan) {
- // InsertTabletPlans contain many rows, each will correspond to a TSStatus as its
+ if (plan instanceof InsertTabletPlan || plan instanceof CreateMultiTimeSeriesPlan) {
+ // InsertTabletPlan contains many rows, each will correspond to a TSStatus as its
Review comment:
Not only `InsertTabletPlan` now, please update the comment.
##########
File path: cluster/src/test/java/org/apache/iotdb/cluster/log/snapshot/DataSnapshotTest.java
##########
@@ -75,6 +75,11 @@ public void readFile(String filePath, long offset, int length,
}
}).start();
}
+
+ @Override
+ public void removeHardLink(String hardLinkPath, AsyncMethodCallback<Void> resultHandler) throws TException {
+ System.out.println("remote hardLinkPath: " + hardLinkPath);
Review comment:
Actually delete the file using something like `Files.deleteIfExists()`.
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
##########
@@ -1937,6 +1937,7 @@ private void removeFullyOverlapFiles(TsFileResource newTsFile, Iterator<TsFileRe
while (iterator.hasNext()) {
TsFileResource existingTsFile = iterator.next();
if (newTsFile.getHistoricalVersions().containsAll(existingTsFile.getHistoricalVersions())
+ && !newTsFile.getHistoricalVersions().equals(existingTsFile.getHistoricalVersions())
Review comment:
This may skip the unclosed TsFile that should be replaced by the pulled file, and I think cluster_new now has properly handled this, so please abort the change.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -1070,11 +1075,19 @@ private boolean createMultiTimeSeries(CreateMultiTimeSeriesPlan createMultiTimeS
try {
createTimeSeries(plan);
} catch (QueryProcessException e) {
- results.put(createMultiTimeSeriesPlan.getIndexes().get(i), e);
+ if (results == null) {
+ results = new TSStatus[createMultiTimeSeriesPlan.getIndexes().size()];
+ Arrays.fill(results, RpcUtils.SUCCESS_STATUS);
+ }
+ results[i] = RpcUtils.getStatus(TSStatusCode.INTERNAL_SERVER_ERROR, e.getMessage());
Review comment:
Use `e.getErrorCode()` here.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -1070,11 +1075,19 @@ private boolean createMultiTimeSeries(CreateMultiTimeSeriesPlan createMultiTimeS
try {
createTimeSeries(plan);
} catch (QueryProcessException e) {
- results.put(createMultiTimeSeriesPlan.getIndexes().get(i), e);
+ if (results == null) {
+ results = new TSStatus[createMultiTimeSeriesPlan.getIndexes().size()];
+ Arrays.fill(results, RpcUtils.SUCCESS_STATUS);
+ }
+ results[i] = RpcUtils.getStatus(TSStatusCode.INTERNAL_SERVER_ERROR, e.getMessage());
+ hasFailed = true;
logger.debug("meet error while processing create timeseries. ", e);
}
}
- createMultiTimeSeriesPlan.setResults(results);
+
+ if (hasFailed) {
+ throw new BatchInsertionException(results);
Review comment:
I think we should rename this to `BatchProcessException` now.
----------------------------------------------------------------
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