Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[IOTDB-1117][Distributed]Batched creation and fetch of RemoteSeriesRe… #2875

Merged
merged 10 commits into from
Mar 25, 2021
Merged

[IOTDB-1117][Distributed]Batched creation and fetch of RemoteSeriesRe… #2875

merged 10 commits into from
Mar 25, 2021

Conversation

wangchao316
Copy link
Member

@wangchao316 wangchao316 commented Mar 19, 2021 •
edited
Loading

  1. Deliver readers of the same partition in batches to form a RemoteMultSeriesReader.
  2. A MultiSeriesRawDataPointReader is created for readers of the same local partition in batches.
  3. Construct an AssignPathManagedMergeReader for multiple partitions of the same sequence and implement it by using the MultPriorityMergeReader class.

Test results show that query glitches are reduced by 30%.

@wangchao316
[IOTDB-1117][Distributed]Batched creation and fetch of RemoteSeriesRe…
a05440b 
…ader
@wangchao316
Merge branch 'master' of https://github.com/apache/iotdb into IOTDB-1117
4392553 


# Conflicts:
#	cluster/src/main/java/org/apache/iotdb/cluster/server/service/DataAsyncService.java
#	cluster/src/main/java/org/apache/iotdb/cluster/server/service/DataSyncService.java
#	cluster/src/test/java/org/apache/iotdb/cluster/common/TestAsyncDataClient.java
@wangchao316
mvn spotless:apply
ff6da51
@wangchao316
add UT
0a3cf2e
jt2594838
jt2594838 requested changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@jt2594838 jt2594838 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, but there are still some small issues. I think the definition and usage of IMultReader are confusing, especially that some classes are defined as Mult but only have Single behavior.

cluster/src/main/java/org/apache/iotdb/cluster/query/reader/ClusterReaderFactory.java Outdated Show resolved Hide resolved
cluster/src/main/java/org/apache/iotdb/cluster/query/reader/ClusterReaderFactory.java Outdated Show resolved Hide resolved
cluster/src/main/java/org/apache/iotdb/cluster/query/reader/ClusterReaderFactory.java Show resolved Hide resolved
cluster/src/main/java/org/apache/iotdb/cluster/query/reader/mult/IMultBatchReader.java Show resolved Hide resolved
cluster/src/main/java/org/apache/iotdb/cluster/query/reader/mult/MultBatchReader.java Outdated Show resolved Hide resolved
cluster/src/main/java/org/apache/iotdb/cluster/query/reader/mult/MultManagedMergeReader.java Outdated Show resolved Hide resolved
cluster/src/main/java/org/apache/iotdb/cluster/query/reader/mult/MultPriorityMergeReader.java Outdated Show resolved Hide resolved
cluster/src/main/java/org/apache/iotdb/cluster/query/reader/mult/RemoteMultSeriesReader.java Outdated Show resolved Hide resolved
cluster/src/main/java/org/apache/iotdb/cluster/query/reader/mult/RemoteMultSeriesReader.java Outdated Show resolved Hide resolved
@OneSizeFitsQuorum OneSizeFitsQuorum added the Module - Cluster PRs for the cluster module label Mar 19, 2021
@wangchao316 wangchao316 marked this pull request as draft March 20, 2021 01:10
@wangchao316 wangchao316 marked this pull request as ready for review March 20, 2021 08:29
@wangchao316
Copy link
Member Author

Good job, but there are still some small issues. I think the definition and usage of IMultReader are confusing, especially that some classes are defined as Mult but only have Single behavior.

I refactor code， please review again.

@wangchao316 wangchao316 marked this pull request as draft March 22, 2021 02:10
@wangchao316 wangchao316 marked this pull request as ready for review March 22, 2021 02:18
@wangchao316
Copy link
Member Author

@mychaow @neuyilan @LebronAl hi, could you please review?
Thanks

jxlgzwh
jxlgzwh suggested changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@jxlgzwh jxlgzwh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to output exception information through logger, don’t ignore

cluster/src/main/java/org/apache/iotdb/cluster/query/LocalQueryExecutor.java Outdated
try {
paths.add(new PartialPath(fullPath));
} catch (IllegalPathException e) {
// ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to output exception information through logger, don’t ignore

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

jxlgzwh
jxlgzwh approved these changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@jxlgzwh jxlgzwh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinks!

@wangchao316
Merge branch 'master' of https://github.com/apache/iotdb into IOTDB-1117
43bf8a2 


# Conflicts:
#	cluster/src/main/java/org/apache/iotdb/cluster/query/ClusterDataQueryExecutor.java
mychaow
mychaow approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@mychaow mychaow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cluster/src/main/java/org/apache/iotdb/cluster/query/ClusterDataQueryExecutor.java
TSDataType dataType = queryPlan.getDeduplicatedDataTypes().get(i);
AssignPathManagedMergeReader assignPathManagedMergeReader =
new AssignPathManagedMergeReader(partialPath.getFullPath(), dataType);
for (AbstractMultPointReader multPointReader : multPointReaders) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think AbstractMultPointReader -> AbstractMultiPointReader is better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mult can mean multiple times,
iPointReader and Multi are easy to confuse,
So Mult can express abstract classes better here.

@neuyilan neuyilan merged commit 910b184 into apache:master Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mychaow mychaow mychaow approved these changes

@jxlgzwh jxlgzwh jxlgzwh approved these changes

@jt2594838 jt2594838 jt2594838 approved these changes

@Alima777 Alima777 Awaiting requested review from Alima777

Assignees
No one assigned
Labels
Module - Cluster PRs for the cluster module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wangchao316 @mychaow @jxlgzwh @jt2594838 @neuyilan @OneSizeFitsQuorum