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.

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

[IOTDB-68] New shared-nothing cluster #460

Merged
merged 1,546 commits into from
Dec 8, 2020
Merged

[IOTDB-68] New shared-nothing cluster #460

merged 1,546 commits into from
Dec 8, 2020

Conversation

qiaojialin
Copy link
Member

No description provided.

@jixuan1989
Copy link
Member

RaftMember.java
Now we have to send request to all nodes and wait for quorum nodes responses. then we can commit the log.

How about:

  1. guarantee there is only one connection between two nodes.
  2. The leader can send the second req to a node after the first req is sent to the node.
  3. However, the leader can commit log index i until i-1 is committed.

@jixuan1989
Copy link
Member

PartitionGroup extends ArrayList is a little heavy..
an array is enough.

@sonarcloud
Copy link

sonarcloud bot commented Feb 7, 2020

SonarCloud Quality Gate failed.

Bug E 28 Bugs
Vulnerability B 11 Vulnerabilities (and Security Hotspot 4 Security Hotspots to review)
Code Smell A 200 Code Smells

No Coverage information No Coverage information
0.3% 0.3% Duplication

@qiaojialin qiaojialin changed the title New shared-nothing cluster [IOTDB-68] New shared-nothing cluster Feb 23, 2020
neuyilan and others added 25 commits October 27, 2020 20:26
fix some regression bugs
[IOTDB-884] group createMultiTimeseriesPlan by partitionGroup
# Conflicts:
#	docs/UserGuide/Server/Cluster Setup.md
#	docs/UserGuide/System Tools/NodeTool.md
#	docs/zh/UserGuide/Server/Cluster Setup.md
#	docs/zh/UserGuide/System Tools/NodeTool.md
#	server/src/main/java/org/apache/iotdb/db/engine/StorageEngine.java
#	server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
#	server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileProcessor.java
#	server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
#	server/src/main/java/org/apache/iotdb/db/metadata/MManager.java
#	server/src/main/java/org/apache/iotdb/db/metadata/MTree.java
#	server/src/main/java/org/apache/iotdb/db/metadata/PartialPath.java
#	server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
#	server/src/main/java/org/apache/iotdb/db/qp/physical/PhysicalPlan.java
#	server/src/main/java/org/apache/iotdb/db/qp/physical/crud/InsertPlan.java
#	server/src/main/java/org/apache/iotdb/db/qp/physical/crud/InsertRowPlan.java
#	server/src/main/java/org/apache/iotdb/db/qp/physical/crud/InsertTabletPlan.java
#	server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
#	server/src/main/java/org/apache/iotdb/db/utils/CommonUtils.java
#	server/src/main/java/org/apache/iotdb/db/utils/SchemaUtils.java
#	server/src/test/java/org/apache/iotdb/db/metadata/MManagerBasicTest.java
#	server/src/test/java/org/apache/iotdb/db/sync/receiver/load/FileLoaderTest.java
#	thrift/src/main/thrift/cluster.thrift
fix handling when client is null
trigger election when leader is down
@jt2594838
Copy link
Contributor

The remaining code smells are duplicated code blocks caused by single inheritence in Java, and there are currently no elegant way to fix them.

@@ -235,6 +235,8 @@ public void run() {
ChunkWriterImpl chunkWriter = (ChunkWriterImpl) ioMessage;
chunkWriter.writeToFileWriter(this.writer);
} else {
this.writer.setMinPlanIndex(memTable.getMinPlanIndex());
Copy link
Member

Choose a reason for hiding this comment

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

why add min/max planIndex on ChunkGroupFooter?

Copy link
Member

Choose a reason for hiding this comment

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

It mainly used for a follower to catch up to the leader when doing snapshot task to filter tsfiles, for details please see https://issues.apache.org/jira/browse/IOTDB-918

@@ -162,7 +163,11 @@ public Statistics getStatistics() {

@Override
public void setFilter(Filter filter) {
this.filter = filter;
if (this.filter == null) {
Copy link
Member

Choose a reason for hiding this comment

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

@JackieTien97 check whether this has some negative effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, i think we should just use the filter to overwrite the former one. The former one is actually a time filter and it has been checked when the PageReader is constructed.

And alse don't forget to change back the MemPageReader

Copy link
Contributor

Choose a reason for hiding this comment

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

I previously found that data of this page will miss the previous filter, meaning some data are not time-filtered. We may remove it later if you are confident.

@@ -19,6 +19,11 @@

-->

# 0.10.x (version-2) -> 0.12.x (version-3)
| PR# | Name | Author | Changes |
Copy link
Member

Choose a reason for hiding this comment

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

need to say why on somewhere.

[Distributed] Make cluster configuration consensus with the single-node
Comment on lines 218 to 219
* <p>
* (1) return IoTDBJDBCResultSet or IoTDBNonAlignJDBCResultSet (2) simply get executed
Copy link
Contributor

Choose a reason for hiding this comment

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

format back

Copy link
Member

Choose a reason for hiding this comment

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

In this way, the format will not be modified when the shortcut keys are used in the idea to automatically format, otherwise, the format will be modified if the shortcut keys are used.

@@ -52,7 +52,7 @@
<maxIndex>10</maxIndex>
</rollingPolicy>
<triggeringPolicy class="ch.qos.logback.core.rolling.SizeBasedTriggeringPolicy">
<maxFileSize>1MB</maxFileSize>
<maxFileSize>10MB</maxFileSize>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need to change the default size of logs

Copy link
Member

Choose a reason for hiding this comment

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

it's ok we use the server‘s logback's configuration as default configuration

fileProcessorMap.remove(timePartition);
}
} catch (Exception e) {
logger.error("Cannot close {}", tsFileResource, e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rethrow the exception and add a finally block to do the remove things

Copy link
Contributor

Choose a reason for hiding this comment

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

The exception will also only be logged in the upper level, so it is fine to do the logging here. When there is any exception, removing it may not be safe anymore, we'd better leave it be.

@@ -56,7 +56,7 @@ public NoCompactionTsFileManagement(String storageGroupName, String storageGroup
if (sequence) {
return new ArrayList<>(sequenceFileTreeSet);
} else {
return unSequenceFileList;
return new ArrayList<>(unSequenceFileList);
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to copy it again here, it won't be changed outside.
Actually it's better that the interface's return type should be changed to Collection

Copy link
Contributor

Choose a reason for hiding this comment

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

But it may be changed indide, making an iteration throws a ConcurrentModificationException.

Comment on lines 272 to 273
ReadWriteIOUtils.write(maxPlanIndex, outputStream);
ReadWriteIOUtils.write(minPlanIndex, outputStream);
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicated with the following lines

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed.

try {
Files.deleteIfExists(file.toPath());
} catch (IOException e) {
logger.warn("TsFile {} cannot be deleted: {}", file, e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

better to rethrow the exception

Copy link
Contributor

Choose a reason for hiding this comment

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

This will break the code structure of many callers, and they can do nothing more than logging it.

Files.deleteIfExists(
fsFactory.getFile(file.getPath() + ModificationFile.FILE_SUFFIX).toPath());
} catch (IOException e) {
logger.warn("ModificationFile {} cannot be deleted: {}", file, e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

try {
Files.deleteIfExists(fsFactory.getFile(file.getPath() + RESOURCE_SUFFIX).toPath());
} catch (IOException e) {
logger.warn("TsFileResource {} cannot be deleted: {}", file, e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Comment on lines +43 to +45
private String measurementAlias = "";
// alias of time series used in SELECT AS
private String tsAlias = null;
private String tsAlias = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we distinguish whether the measurement has an empty string as its alias or it doesn't have alias?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Alima777 PTAL(do you think it's a problem?)

Copy link
Member

Choose a reason for hiding this comment

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

as far as I know, iotdb does not support empty string as an alias, so it's ok to use an empty string here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do not support empty strings within timeseries paths, so there is even less meaning to support it in alias, so both make no difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my perspective, it's fine.

@@ -162,7 +163,11 @@ public Statistics getStatistics() {

@Override
public void setFilter(Filter filter) {
this.filter = filter;
if (this.filter == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, i think we should just use the filter to overwrite the former one. The former one is actually a time filter and it has been checked when the PageReader is constructed.

And alse don't forget to change back the MemPageReader

}

@Override
public boolean isOpen() { return true; }
Copy link
Member

Choose a reason for hiding this comment

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

how about set to false once close() is called.

Copy link
Contributor

Choose a reason for hiding this comment

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

No point. close() means nothing to such classes. You may find similar codes in other thrift Transports like MemTransport.

# Conflicts:
#	server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
# Conflicts:
#	server/src/main/java/org/apache/iotdb/db/sync/sender/transfer/SyncClient.java
@sonarcloud
Copy link

sonarcloud bot commented Dec 7, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 11 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

warning The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 11.
Read more here

@jixuan1989
Copy link
Member

Though there are many works left to do for this issue, I have to say you have reached a good milestone. So, I will merge this PR tonight.
Nice work, guys!

@jixuan1989 jixuan1989 merged commit 580b012 into master Dec 8, 2020
kezhenxu94 pushed a commit to kezhenxu94/iotdb that referenced this pull request Dec 13, 2020
Co-authored-by: HouliangQi <neuyilan@163.com>
Co-authored-by: wangyanhong <1101967410@qq.com>
Co-authored-by: chaow <runhuster@foxmail.com>
Co-authored-by: Jiang Tian <jt2594838@163.com>
Co-authored-by: chaow <xunmengzhuiyi@gmail.com>
Co-authored-by: LebronAl <TXYPotato@gmail.com>
Co-authored-by: Ring-k <yuyuankang@hotmail.com>
Co-authored-by: xiangdong huang <sainthxd@gmail.com>
@qiaojialin qiaojialin deleted the cluster_new branch June 16, 2021 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module - Cluster PRs for the cluster module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet