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/03/19 01:08:13 UTC
[GitHub] [iotdb] jxlgzwh opened a new pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
jxlgzwh opened a new pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872
Improve jdbc performance for creating timeseries in cluster module
--
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] [iotdb] jxlgzwh commented on pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
Posted by GitBox <gi...@apache.org>.
jxlgzwh commented on pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872#issuecomment-806374983
@neuyilan @jixuan1989 @jt2594838 PTAL again. Thanks!
--
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] [iotdb] jxlgzwh commented on a change in pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
Posted by GitBox <gi...@apache.org>.
jxlgzwh commented on a change in pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872#discussion_r600095421
##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -506,6 +534,30 @@ public TSStatus executeBatchStatement(TSExecuteBatchStatementReq req) {
}
}
}
+ if (paths.size() > 0) {
+ multiPlan.setPaths(paths);
+ multiPlan.setDataTypes(tsDataTypes);
+ multiPlan.setEncodings(tsEncodings);
+ multiPlan.setCompressors(tsCompressionTypes);
+ multiPlan.setTags(tagsList);
+ multiPlan.setAttributes(attributesList);
+ multiPlan.setAlias(aliasList);
+ TSStatus status = executeNonQueryPlan(multiPlan);
+ if (status.code != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ int startIndex = result.size();
+ if (startIndex > 0) {
+ startIndex = startIndex - 1;
+ }
+ for (int i = 0; i < paths.size(); i++) {
+ result.add(RpcUtils.SUCCESS_STATUS);
+ }
+ for (Entry<Integer, TSStatus> entry : multiPlan.getResults().entrySet()) {
+ result.set(startIndex + entry.getKey(), entry.getValue());
+ }
+ isAllSuccessful = false;
+ }
+ }
+
Measurement.INSTANCE.addOperationLatency(Operation.EXECUTE_JDBC_BATCH, t1);
return isAllSuccessful
? RpcUtils.getStatus(TSStatusCode.SUCCESS_STATUS, "Execute batch statements successfully")
Review comment:
already 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] [iotdb] jxlgzwh commented on a change in pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
Posted by GitBox <gi...@apache.org>.
jxlgzwh commented on a change in pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872#discussion_r600114834
##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -461,6 +461,16 @@ public TSStatus executeBatchStatement(TSExecuteBatchStatementReq req) {
InsertRowsPlan insertRowsPlan = new InsertRowsPlan();
int index = 0;
+
+ CreateMultiTimeSeriesPlan multiPlan = new CreateMultiTimeSeriesPlan();
Review comment:
Changes completed
--
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] [iotdb] neuyilan commented on a change in pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
Posted by GitBox <gi...@apache.org>.
neuyilan commented on a change in pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872#discussion_r598080962
##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -506,6 +534,30 @@ public TSStatus executeBatchStatement(TSExecuteBatchStatementReq req) {
}
}
}
+ if (paths.size() > 0) {
+ multiPlan.setPaths(paths);
+ multiPlan.setDataTypes(tsDataTypes);
+ multiPlan.setEncodings(tsEncodings);
+ multiPlan.setCompressors(tsCompressionTypes);
+ multiPlan.setTags(tagsList);
+ multiPlan.setAttributes(attributesList);
+ multiPlan.setAlias(aliasList);
+ TSStatus status = executeNonQueryPlan(multiPlan);
+ if (status.code != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ int startIndex = result.size();
+ if (startIndex > 0) {
+ startIndex = startIndex - 1;
+ }
+ for (int i = 0; i < paths.size(); i++) {
+ result.add(RpcUtils.SUCCESS_STATUS);
+ }
+ for (Entry<Integer, TSStatus> entry : multiPlan.getResults().entrySet()) {
+ result.set(startIndex + entry.getKey(), entry.getValue());
+ }
+ isAllSuccessful = false;
+ }
+ }
+
Measurement.INSTANCE.addOperationLatency(Operation.EXECUTE_JDBC_BATCH, t1);
return isAllSuccessful
? RpcUtils.getStatus(TSStatusCode.SUCCESS_STATUS, "Execute batch statements successfully")
Review comment:
We can not do the meta operation in the last step, for example, we have the following SQL statements:
example1:
> delete timeseries root.ln.wf01.wt01.status
> insert into root.ln.wf01.wt01(timestamp,status) values(1,true)
example2:
> create timeseries root.ln.wf01.wt01.temperature with datatype=FLOAT,encoding=RLE
> create timeseries root.ln.wf01.wt01.status with datatype=BOOLEAN,encoding=PLAIN
> // insert one record with wrong type
> insert into root.ln.wf01.wt01(timestamp,status) values(1,"this is wrong type, can not be insert success")
For example 1, if we first execute the insert statement, then execute the delete statement, all data will be deleted.
For example 2, if we first execute the insert statement, IoTDB will create **TEXT** type for root.ln.wf01.wt01.status automatically, which is in conflict with the user given type(**BOOLEN**).
--
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] [iotdb] jt2594838 commented on a change in pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
Posted by GitBox <gi...@apache.org>.
jt2594838 commented on a change in pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872#discussion_r600209938
##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -459,8 +537,11 @@ public TSStatus executeBatchStatement(TSExecuteBatchStatementReq req) {
return RpcUtils.getStatus(TSStatusCode.NOT_LOGIN_ERROR);
}
- InsertRowsPlan insertRowsPlan = new InsertRowsPlan();
+ InsertRowsPlan insertRowsPlan;
int index = 0;
+ ArrayList executeList = new ArrayList();
Review comment:
It would be better to define a variable using interface type.
##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -472,19 +553,47 @@ public TSStatus executeBatchStatement(TSExecuteBatchStatementReq req) {
}
if (physicalPlan.getOperatorType().equals(OperatorType.INSERT)) {
+ if (OperatorType.INSERT == lastOperatorType) {
+ insertRowsPlan = (InsertRowsPlan) executeList.get(executeList.size() - 1);
+ } else {
+ insertRowsPlan = new InsertRowsPlan();
+ executeList.add(insertRowsPlan);
+ index = 0;
+ }
+ lastOperatorType = OperatorType.INSERT;
insertRowsPlan.addOneInsertRowPlan((InsertRowPlan) physicalPlan, index);
index++;
- if (i == req.getStatements().size() - 1
- && !executeInsertRowsPlan(insertRowsPlan, result)) {
- isAllSuccessful = false;
+ if (i == req.getStatements().size() - 1) {
+ if (!executeBatchList(executeList, result)) {
+ isAllSuccessful = false;
+ }
+ executeList = new ArrayList();
Review comment:
Is it necessary to create a new list now that you have processed the last statement?
And maybe this branch can be extracted from the loop.
##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -472,19 +553,47 @@ public TSStatus executeBatchStatement(TSExecuteBatchStatementReq req) {
}
if (physicalPlan.getOperatorType().equals(OperatorType.INSERT)) {
+ if (OperatorType.INSERT == lastOperatorType) {
+ insertRowsPlan = (InsertRowsPlan) executeList.get(executeList.size() - 1);
+ } else {
+ insertRowsPlan = new InsertRowsPlan();
+ executeList.add(insertRowsPlan);
+ index = 0;
+ }
+ lastOperatorType = OperatorType.INSERT;
insertRowsPlan.addOneInsertRowPlan((InsertRowPlan) physicalPlan, index);
index++;
- if (i == req.getStatements().size() - 1
- && !executeInsertRowsPlan(insertRowsPlan, result)) {
- isAllSuccessful = false;
+ if (i == req.getStatements().size() - 1) {
+ if (!executeBatchList(executeList, result)) {
+ isAllSuccessful = false;
+ }
+ executeList = new ArrayList();
+ }
+ } else if (physicalPlan.getOperatorType().equals(OperatorType.CREATE_TIMESERIES)) {
+ if (OperatorType.CREATE_TIMESERIES == lastOperatorType) {
+ multiPlan = (CreateMultiTimeSeriesPlan) executeList.get(executeList.size() - 1);
+ } else {
+ multiPlan = new CreateMultiTimeSeriesPlan();
+ executeList.add(multiPlan);
+ }
+ lastOperatorType = OperatorType.CREATE_TIMESERIES;
+ initMultiTimeSeriesPlan(multiPlan);
+
+ CreateTimeSeriesPlan createTimeSeriesPlan = (CreateTimeSeriesPlan) physicalPlan;
+ setMultiTimeSeriesPlan(multiPlan, createTimeSeriesPlan);
+ if (i == req.getStatements().size() - 1) {
+ if (!executeBatchList(executeList, result)) {
+ isAllSuccessful = false;
+ }
+ executeList = new ArrayList();
}
} else {
- if (insertRowsPlan.getRowCount() > 0) {
- if (!executeInsertRowsPlan(insertRowsPlan, result)) {
+ lastOperatorType = physicalPlan.getOperatorType();
+ if (executeList.size() > 0) {
+ if (!executeBatchList(executeList, result)) {
isAllSuccessful = false;
}
- index = 0;
- insertRowsPlan = new InsertRowsPlan();
+ executeList = new ArrayList();
Review comment:
`List.clear` should be enough here.
##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -447,7 +447,85 @@ private boolean executeInsertRowsPlan(InsertRowsPlan insertRowsPlan, List<TSStat
result.set(startIndex + entry.getKey(), entry.getValue());
}
}
- return tsStatus.equals(RpcUtils.SUCCESS_STATUS);
+ return tsStatus.getCode() == RpcUtils.SUCCESS_STATUS.getCode();
+ }
+
+ private boolean executeMultiTimeSeriesPlan(
+ CreateMultiTimeSeriesPlan multiPlan, List<TSStatus> result) {
+ long t1 = System.currentTimeMillis();
+ TSStatus tsStatus = executeNonQueryPlan(multiPlan);
+ Measurement.INSTANCE.addOperationLatency(Operation.EXECUTE_ROWS_PLAN_IN_BATCH, t1);
+
+ int startIndex = result.size();
+ if (startIndex > 0) {
+ startIndex = startIndex - 1;
+ }
+ for (int k = 0; k < multiPlan.getPaths().size(); k++) {
+ result.add(RpcUtils.SUCCESS_STATUS);
+ }
+ if (tsStatus.subStatus != null) {
+ for (Entry<Integer, TSStatus> entry : multiPlan.getResults().entrySet()) {
+ result.set(startIndex + entry.getKey(), entry.getValue());
+ }
+ }
+ return tsStatus.getCode() == RpcUtils.SUCCESS_STATUS.getCode();
+ }
+
+ private void initMultiTimeSeriesPlan(CreateMultiTimeSeriesPlan multiPlan) {
+ if (multiPlan.getPaths() == null) {
+ List<PartialPath> paths = new ArrayList<>();
+ List<TSDataType> tsDataTypes = new ArrayList<>();
+ List<TSEncoding> tsEncodings = new ArrayList<>();
+ List<CompressionType> tsCompressionTypes = new ArrayList<>();
+ List<Map<String, String>> tagsList = new ArrayList<>();
+ List<Map<String, String>> attributesList = new ArrayList<>();
+ List<String> aliasList = new ArrayList<>();
+ multiPlan.setPaths(paths);
+ multiPlan.setDataTypes(tsDataTypes);
+ multiPlan.setEncodings(tsEncodings);
+ multiPlan.setCompressors(tsCompressionTypes);
+ multiPlan.setTags(tagsList);
+ multiPlan.setAttributes(attributesList);
+ multiPlan.setAlias(aliasList);
+ }
+ }
+
+ private void setMultiTimeSeriesPlan(
+ CreateMultiTimeSeriesPlan multiPlan, CreateTimeSeriesPlan createTimeSeriesPlan) {
+ PartialPath path = createTimeSeriesPlan.getPath();
+ TSDataType type = createTimeSeriesPlan.getDataType();
+ TSEncoding encoding = createTimeSeriesPlan.getEncoding();
+ CompressionType compressor = createTimeSeriesPlan.getCompressor();
+ Map<String, String> tags = createTimeSeriesPlan.getTags();
+ Map<String, String> attributes = createTimeSeriesPlan.getAttributes();
+ String alias = createTimeSeriesPlan.getAlias();
+
+ multiPlan.getPaths().add(path);
+ multiPlan.getDataTypes().add(type);
+ multiPlan.getEncodings().add(encoding);
+ multiPlan.getCompressors().add(compressor);
+ multiPlan.getTags().add(tags);
+ multiPlan.getAttributes().add(attributes);
+ multiPlan.getAlias().add(alias);
+ }
+
+ private boolean executeBatchList(ArrayList executeList, List<TSStatus> result) {
+ boolean isAllSuccessful = true;
+ if (executeList.size() > 0) {
+ for (int j = 0; j < executeList.size(); j++) {
Review comment:
`if (executeList.size() > 0) {` can be removed.
##########
File path: server/src/test/java/org/apache/iotdb/db/integration/IoTDBExecuteBatchIT.java
##########
@@ -26,11 +26,7 @@
import org.junit.Before;
import org.junit.Test;
-import java.sql.Connection;
-import java.sql.DriverManager;
-import java.sql.ResultSet;
-import java.sql.SQLException;
-import java.sql.Statement;
+import java.sql.*;
Review comment:
I think we prefer not to use wildcards in imports.
--
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] [iotdb] jxlgzwh edited a comment on pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
Posted by GitBox <gi...@apache.org>.
jxlgzwh edited a comment on pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872#issuecomment-805415386
I found an error in the executeInsertRowsPlan method of https://github.com/apache/iotdb/pull/2725
tsStatus.equals(RpcUtils.SUCCESS_STATUS)
This method will always return false because
The message of tsStatus has a set value, so isSetMessage is true
The message of RpcUtils.SUCCESS_STATUS is not set, so isSetMessage is false
tsStatus.equals(RpcUtils.SUCCESS_STATUS) will return false
So I modified it to compare the code and fix it together with pr this time
--
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] [iotdb] mychaow merged pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
Posted by GitBox <gi...@apache.org>.
mychaow merged pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872
--
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] [iotdb] jxlgzwh commented on pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
Posted by GitBox <gi...@apache.org>.
jxlgzwh commented on pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872#issuecomment-805415386
I found an error in the executeInsertRowsPlan method of https://github.com/apache/iotdb/pull/2725
tsStatus.equals(RpcUtils.SUCCESS_STATUS)
This method will always return false because
The message of tsStatus has a set value, so isSetMessage is true
The message of RpcUtils.SUCCESS_STATUS is not set, so isSetMessage is false
So I modified it to compare the code and fix it together with pr this time
--
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] [iotdb] jixuan1989 commented on a change in pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872#discussion_r597395725
##########
File path: server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
##########
@@ -461,6 +461,16 @@ public TSStatus executeBatchStatement(TSExecuteBatchStatementReq req) {
InsertRowsPlan insertRowsPlan = new InsertRowsPlan();
int index = 0;
+
+ CreateMultiTimeSeriesPlan multiPlan = new CreateMultiTimeSeriesPlan();
Review comment:
you cannot create so many Object instances before you confirm some statements can be wrapped by a CreateMultiTimeSeriesPlan.
--
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] [iotdb] jxlgzwh commented on pull request #2872: [IOTDB-1236] Improve jdbc performance for creating timeseries in cluster module
Posted by GitBox <gi...@apache.org>.
jxlgzwh commented on pull request #2872:
URL: https://github.com/apache/iotdb/pull/2872#issuecomment-805411219
> 1. Add Tests for new codes
> 2. Describe your design or idea in the PR.
Changes completed
--
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