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 2021/02/25 02:15:21 UTC
[GitHub] [iotdb] neuyilan commented on a change in pull request #2731: [IOTDB-1173] auto-create schema improvement for createMultiTimeSeriesPlan
neuyilan commented on a change in pull request #2731:
URL: https://github.com/apache/iotdb/pull/2731#discussion_r582436686
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -426,58 +428,77 @@ public synchronized boolean containsKey(PartialPath key) {
}
/**
- * create storage groups for CreateTimeseriesPlan and InsertPlan, also create timeseries for
- * InsertPlan. Only the two kind of plans can use this method.
+ * create storage groups for CreateTimeseriesPlan, CreateMultiTimeseriesPlan and InsertPlan, also
+ * create timeseries for InsertPlan. Only the three kind of plans can use this method.
*/
public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsistencyException {
- // try to set storage group
- List<PartialPath> deviceIds;
- // only handle InsertPlan, CreateTimeSeriesPlan and CreateMultiTimeSeriesPlan currently,
- if (plan instanceof InsertPlan
- && !(plan instanceof InsertMultiTabletPlan)
- && !(plan instanceof InsertRowsPlan)) {
- // InsertMultiTabletPlan and InsertRowsPlan have multiple devices, and other types of
- // InsertPlan have only one device.
- deviceIds = Collections.singletonList(((InsertPlan) plan).getDeviceId());
+ List<PartialPath> storageGroups = new ArrayList<>();
+ // for InsertPlan, try to just use deviceIds to get related storage groups because there's no
+ // need to call getPaths to concat deviceId and sensor as they will gain same result.
+ // for CreateTimeSeriesPlan, use getPath() to get timeseries to get related storage group
+ // for CreateTimeMultiSeriesPlan, use getPaths() to get all timeseries to get related storage
+ // groups
Review comment:
CreateTimeMultiSeriesPlan -> CreateMultiTimeSeriesPlan
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -426,58 +428,77 @@ public synchronized boolean containsKey(PartialPath key) {
}
/**
- * create storage groups for CreateTimeseriesPlan and InsertPlan, also create timeseries for
- * InsertPlan. Only the two kind of plans can use this method.
+ * create storage groups for CreateTimeseriesPlan, CreateMultiTimeseriesPlan and InsertPlan, also
+ * create timeseries for InsertPlan. Only the three kind of plans can use this method.
*/
public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsistencyException {
- // try to set storage group
- List<PartialPath> deviceIds;
- // only handle InsertPlan, CreateTimeSeriesPlan and CreateMultiTimeSeriesPlan currently,
- if (plan instanceof InsertPlan
- && !(plan instanceof InsertMultiTabletPlan)
- && !(plan instanceof InsertRowsPlan)) {
- // InsertMultiTabletPlan and InsertRowsPlan have multiple devices, and other types of
- // InsertPlan have only one device.
- deviceIds = Collections.singletonList(((InsertPlan) plan).getDeviceId());
+ List<PartialPath> storageGroups = new ArrayList<>();
+ // for InsertPlan, try to just use deviceIds to get related storage groups because there's no
+ // need to call getPaths to concat deviceId and sensor as they will gain same result.
+ // for CreateTimeSeriesPlan, use getPath() to get timeseries to get related storage group
Review comment:
```suggestion
// for CreateTimeSeriesPlan, use getPath() to get timeseries to get related storage group,
```
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -500,24 +521,23 @@ private void verifyCreatedSgSuccess(List<PartialPath> deviceIds, PhysicalPlan ph
}
/**
- * Create storage group automatically for deviceId.
+ * Create storage groups automatically for paths.
*
- * @param deviceId
+ * @param storageGroups the uncreated storage groups
*/
- private void createStorageGroup(PartialPath deviceId) throws MetadataException {
- PartialPath storageGroupName =
- MetaUtils.getStorageGroupPathByLevel(
- deviceId, IoTDBDescriptor.getInstance().getConfig().getDefaultStorageGroupLevel());
- SetStorageGroupPlan setStorageGroupPlan = new SetStorageGroupPlan(storageGroupName);
- TSStatus setStorageGroupResult =
- metaGroupMember.processNonPartitionedMetaPlan(setStorageGroupPlan);
- if (setStorageGroupResult.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()
- && setStorageGroupResult.getCode()
- != TSStatusCode.PATH_ALREADY_EXIST_ERROR.getStatusCode()) {
- throw new MetadataException(
- String.format(
- "Status Code: %d, failed to set storage group %s",
- setStorageGroupResult.getCode(), storageGroupName));
+ private void createStorageGroups(List<PartialPath> storageGroups) throws MetadataException {
+ for (PartialPath storageGroup : storageGroups) {
+ SetStorageGroupPlan setStorageGroupPlan = new SetStorageGroupPlan(storageGroup);
+ TSStatus setStorageGroupResult =
+ metaGroupMember.processNonPartitionedMetaPlan(setStorageGroupPlan);
+ if (setStorageGroupResult.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()
+ && setStorageGroupResult.getCode()
+ != TSStatusCode.PATH_ALREADY_EXIST_ERROR.getStatusCode()) {
+ throw new MetadataException(
+ String.format(
+ "Status Code: %d, failed to set storage group %s",
+ setStorageGroupResult.getCode(), storageGroup));
+ }
Review comment:
Hi, do we need to allow partial set storage group success? @LebronAl @mychaow @jt2594838
In the current implementation, if one of the storage groups is created failure, the remaining storage group will not be created.
----------------------------------------------------------------
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