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/09/26 04:09:09 UTC
[GitHub] [iotdb] ericpai opened a new pull request #4034: Cache paths list in batched insert plan
ericpai opened a new pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034
See JIRA https://issues.apache.org/jira/browse/IOTDB-1738
--
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 #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#discussion_r716178350
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -563,10 +569,11 @@ public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsi
private List<PartialPath> getValidStorageGroups(PhysicalPlan plan) {
List<PartialPath> paths = new ArrayList<>();
- for (int i = 0; i < plan.getPaths().size(); i++) {
+ List<PartialPath> originalPaths = plan.getPaths();
+ for (int i = 0; i < originalPaths.size(); i++) {
Review comment:
I have pushed a new commit. It includes:
1. Fix this issue.
2. Cache getPrefixPath.
3. Leave and move the de-duplicate logic to BasicApplier, which may help to speed up the process by former design.
--
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] neuyilan commented on a change in pull request #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
neuyilan commented on a change in pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#discussion_r716167176
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -563,10 +569,11 @@ public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsi
private List<PartialPath> getValidStorageGroups(PhysicalPlan plan) {
List<PartialPath> paths = new ArrayList<>();
- for (int i = 0; i < plan.getPaths().size(); i++) {
+ List<PartialPath> originalPaths = plan.getPaths();
+ for (int i = 0; i < originalPaths.size(); i++) {
Review comment:
The `(BatchPlan) plan).getResults().containsKey(i)` here are those plans that do not have authority to operate, so here we do not process those plans.
--
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] neuyilan commented on a change in pull request #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
neuyilan commented on a change in pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#discussion_r716177057
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -563,10 +569,11 @@ public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsi
private List<PartialPath> getValidStorageGroups(PhysicalPlan plan) {
List<PartialPath> paths = new ArrayList<>();
- for (int i = 0; i < plan.getPaths().size(); i++) {
+ List<PartialPath> originalPaths = plan.getPaths();
+ for (int i = 0; i < originalPaths.size(); i++) {
Review comment:
> And I think the prefixPaths should be cached in the plan as well
It's ok for me
--
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 #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#discussion_r716168746
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -563,10 +569,11 @@ public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsi
private List<PartialPath> getValidStorageGroups(PhysicalPlan plan) {
List<PartialPath> paths = new ArrayList<>();
- for (int i = 0; i < plan.getPaths().size(); i++) {
+ List<PartialPath> originalPaths = plan.getPaths();
+ for (int i = 0; i < originalPaths.size(); i++) {
Review comment:
The variable `i` is the index of plan.getPaths(), not the sub insert plans.
Let's take an InsertRowsPlan as example, which equals `insert into root.sg.d1(time, s1, s2, s3) values(1, 1, 2, 3); insert into(time, s1, s2, s3) values(2, 4, 5, 6);`
1. The plan.getPaths() returns `[root.sg.d1.s1, root.sg.d1.s2, root.d1.s3]` (the order is not deterministic as it's a set to list). The `insertRowPlanList` contains two elements `(InsertRowPlan, [s1,s2,s3], [1,2,3])` and `(InsertRowPlan, [s1,s2,s3], [4,5,6])`.
2. The getResults() map may only contain key `0` and `1` as there're only 2 InsertRowPlan.
3. `i` is the path index generated from getPaths() list, and there's no relationship with the plan index.
--
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 #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
ericpai commented on pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#issuecomment-928606437
@neuyilan Is this ready to be merged? I think maybe rel/0.12 have the same issue
--
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] neuyilan commented on a change in pull request #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
neuyilan commented on a change in pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#discussion_r716174057
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -563,10 +569,11 @@ public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsi
private List<PartialPath> getValidStorageGroups(PhysicalPlan plan) {
List<PartialPath> paths = new ArrayList<>();
- for (int i = 0; i < plan.getPaths().size(); i++) {
+ List<PartialPath> originalPaths = plan.getPaths();
+ for (int i = 0; i < originalPaths.size(); i++) {
Review comment:
Yes, I find only `private void pullTimeseriesSchema(InsertPlan plan, RaftNode ignoredGroup)` method in `BaseAppIier` class used the `getPrefixPaths() `methods and it have logic combine the same sg when pull timeseries, so I think we can just change the `Set` to `List`, how 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
[GitHub] [iotdb] neuyilan commented on pull request #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
neuyilan commented on pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#issuecomment-928610092
> @neuyilan Is this ready to be merged? I think maybe rel/0.12 have the same issue
Yes, better merged to the rel/0.12 branch.
--
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] neuyilan commented on pull request #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
neuyilan commented on pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#issuecomment-928610092
> @neuyilan Is this ready to be merged? I think maybe rel/0.12 have the same issue
Yes, better merged to the rel/0.12 branch.
--
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] neuyilan commented on a change in pull request #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
neuyilan commented on a change in pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#discussion_r716171938
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -563,10 +569,11 @@ public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsi
private List<PartialPath> getValidStorageGroups(PhysicalPlan plan) {
List<PartialPath> paths = new ArrayList<>();
- for (int i = 0; i < plan.getPaths().size(); i++) {
+ List<PartialPath> originalPaths = plan.getPaths();
+ for (int i = 0; i < originalPaths.size(); i++) {
Review comment:
Thanks for point out.
Yes, this is one bug, it should be used `getPrefixPath()` rather than the` plan.getPaths()`
`for (int i = 0; i < plan.getPrefixPath().size(); i++)
...
`
--
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] neuyilan merged pull request #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
neuyilan merged pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034
--
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] neuyilan merged pull request #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
neuyilan merged pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034
--
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 #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
ericpai commented on pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#issuecomment-927496893
@neuyilan Please review these changes again. I find another issue in `CreateMultiTimeseriesPlan.getPrefixPaths()` and have fixed it.
--
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 #4034: Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#discussion_r716136804
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -563,10 +569,11 @@ public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsi
private List<PartialPath> getValidStorageGroups(PhysicalPlan plan) {
List<PartialPath> paths = new ArrayList<>();
- for (int i = 0; i < plan.getPaths().size(); i++) {
+ List<PartialPath> originalPaths = plan.getPaths();
+ for (int i = 0; i < originalPaths.size(); i++) {
Review comment:
Here I have some questions about this method. The key of getResults() is the index of the insertRowPlan, not the path. Is this statement a bug?
--
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 #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#discussion_r716173217
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -563,10 +569,11 @@ public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsi
private List<PartialPath> getValidStorageGroups(PhysicalPlan plan) {
List<PartialPath> paths = new ArrayList<>();
- for (int i = 0; i < plan.getPaths().size(); i++) {
+ List<PartialPath> originalPaths = plan.getPaths();
+ for (int i = 0; i < originalPaths.size(); i++) {
Review comment:
However, in InsertRowsPlan's getPrefixPaths(), it uses a set to deduplicate the prefix paths, which losing the prefixPath indexing information for using here. I think it can't resolve this issue.
--
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 #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
ericpai commented on pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#issuecomment-928606437
@neuyilan Is this ready to be merged? I think maybe rel/0.12 have the same issue
--
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 #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#discussion_r716172502
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -563,10 +569,11 @@ public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsi
private List<PartialPath> getValidStorageGroups(PhysicalPlan plan) {
List<PartialPath> paths = new ArrayList<>();
- for (int i = 0; i < plan.getPaths().size(); i++) {
+ List<PartialPath> originalPaths = plan.getPaths();
+ for (int i = 0; i < originalPaths.size(); i++) {
Review comment:
That makes sense! And I think prefixPaths should be cached as well. 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
[GitHub] [iotdb] ericpai commented on a change in pull request #4034: [IOTDB-1738] Cache paths list in batched insert plan
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #4034:
URL: https://github.com/apache/iotdb/pull/4034#discussion_r716175390
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -563,10 +569,11 @@ public void createSchema(PhysicalPlan plan) throws MetadataException, CheckConsi
private List<PartialPath> getValidStorageGroups(PhysicalPlan plan) {
List<PartialPath> paths = new ArrayList<>();
- for (int i = 0; i < plan.getPaths().size(); i++) {
+ List<PartialPath> originalPaths = plan.getPaths();
+ for (int i = 0; i < originalPaths.size(); i++) {
Review comment:
That's right! And I think the prefixPaths should be cached in the plan as well
--
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