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 2022/05/06 10:39:36 UTC
[GitHub] [iotdb] cigarl opened a new pull request, #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
cigarl opened a new pull request, #5816:
URL: https://github.com/apache/iotdb/pull/5816
Attributes in `ConfigRequestExecutor` need to implement the `Snapshot` interface and operate the snapshot concurrently.
The current PR focus on `ClusterSchemaInfo`, and the other attributes will be updated along with the PR that implements its serialization.
--
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] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867337626
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/Snapshot.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.confignode.persistence;
+
+import org.apache.thrift.TException;
+
+import java.io.File;
+import java.io.IOException;
+
+public interface Snapshot {
Review Comment:
OK，let me update the name and add some comments
--
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] wangchao316 commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
wangchao316 commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867317113
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/executor/ConfigRequestExecutor.java:
##########
@@ -138,4 +151,66 @@ public TSStatus executorNonQueryPlan(ConfigRequest req)
throw new UnknownPhysicalPlanTypeException(req.getType());
}
}
+
+ public boolean takeSnapshot(File snapshotDir) {
Review Comment:
this method use for?
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/executor/ConfigRequestExecutor.java:
##########
@@ -56,6 +67,8 @@ public class ConfigRequestExecutor {
private final AuthorInfo authorInfo;
+ private static final Logger LOGGER = LoggerFactory.getLogger(ConfigRequestExecutor.class);
Review Comment:
generate, Logger add first line in class
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -332,12 +342,59 @@ public List<TConsensusGroupId> getRegionGroupIds(String storageGroup, TConsensus
return result;
}
- public void serialize(ByteBuffer buffer) {
- // TODO: Serialize ClusterSchemaInfo
+ @Override
+ public boolean takeSnapshot(File snapshotDir) throws IOException {
+
+ File snapshotFile = new File(snapshotDir, snapshotFileName);
+ if (snapshotFile.exists() && snapshotFile.isFile()) {
+ LOGGER.error(
+ "Failed to take snapshot, because snapshot file [{}] is already exist.",
+ snapshotFile.getAbsolutePath());
+ return false;
+ }
+
+ File tmpFile = new File(snapshotFile.getAbsolutePath() + "-" + UUID.randomUUID());
+ ByteBuffer buffer = ByteBuffer.allocate(bufferSize);
+
+ storageGroupReadWriteLock.readLock().lock();
Review Comment:
is this write lock ? if data wirte now, we need lock.
--
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] chengjianyun commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867339335
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -43,23 +43,33 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.UUID;
import java.util.concurrent.locks.ReentrantReadWriteLock;
-public class ClusterSchemaInfo {
+public class ClusterSchemaInfo implements SnapshotProcessor {
private static final Logger LOGGER = LoggerFactory.getLogger(ClusterSchemaInfo.class);
// StorageGroup read write lock
private final ReentrantReadWriteLock storageGroupReadWriteLock;
- // TODO: serialize and deserialize
private MTreeAboveSG mTree;
+ // The size of the buffer used for snapshot(temporary value)
+ private final int bufferSize = 10 * 1024 * 1024;
+
+ private final String snapshotFileName = "ClusterSchemaInfo.st";
Review Comment:
ClusterSchemaInfo is much like a class name and looks like IoTDB names file by camel naming pattern. How about use "cluster_schema.bin" as the file name.
--
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] chengjianyun commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r866720761
##########
confignode/src/main/java/org/apache/iotdb/confignode/service/executor/ConfigRequestExecutor.java:
##########
@@ -135,4 +148,56 @@ public TSStatus executorNonQueryPlan(ConfigRequest req)
throw new UnknownPhysicalPlanTypeException(req.getType());
}
}
+
+ public boolean takeSnapshot(File snapshotDir) {
+
Review Comment:
check snapshotDir and mkdirs if doesn't exist.
--
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] chengjianyun commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r866720979
##########
confignode/src/main/java/org/apache/iotdb/confignode/service/executor/ConfigRequestExecutor.java:
##########
@@ -135,4 +148,56 @@ public TSStatus executorNonQueryPlan(ConfigRequest req)
throw new UnknownPhysicalPlanTypeException(req.getType());
}
}
+
+ public boolean takeSnapshot(File snapshotDir) {
+
+ AtomicBoolean result = new AtomicBoolean(true);
+ getAllAttributes()
+ .parallelStream()
+ .forEach(
+ x -> {
+ boolean takeSnapshotResult = true;
+ try {
+ String fileName = x.getClass().getSimpleName();
+ File file = new File(snapshotDir, fileName);
+ takeSnapshotResult = x.takeSnapshot(file);
+ } catch (TException | IOException e) {
+ LOGGER.error(e.getMessage());
+ takeSnapshotResult = false;
+ } finally {
+ // If any snapshot fails, the whole fails
+ // So this is just going to be false
+ if (!takeSnapshotResult) {
+ result.set(false);
+ }
+ }
+ });
+ return result.get();
+ }
+
+ public void loadSnapshot(File latestSnapshotRootDir) {
+
Review Comment:
check dir first.
--
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] OneSizeFitsQuorum commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867336585
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/Snapshot.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.confignode.persistence;
+
+import org.apache.thrift.TException;
+
+import java.io.File;
+import java.io.IOException;
+
+public interface Snapshot {
Review Comment:
If you think they have a different function you can change the name or add a comment, otherwise it will be ambiguous
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/Snapshot.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.confignode.persistence;
+
+import org.apache.thrift.TException;
+
+import java.io.File;
+import java.io.IOException;
+
+public interface Snapshot {
Review Comment:
If you think they have a different function you can change the name or add a comment, otherwise it will be ambiguous
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/executor/ConfigRequestExecutor.java:
##########
@@ -44,10 +44,23 @@
import org.apache.iotdb.confignode.persistence.ClusterSchemaInfo;
import org.apache.iotdb.confignode.persistence.NodeInfo;
import org.apache.iotdb.confignode.persistence.PartitionInfo;
+import org.apache.iotdb.confignode.persistence.Snapshot;
import org.apache.iotdb.consensus.common.DataSet;
+import org.apache.thrift.TException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
public class ConfigRequestExecutor {
Review Comment:
OK
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/executor/ConfigRequestExecutor.java:
##########
@@ -44,10 +44,23 @@
import org.apache.iotdb.confignode.persistence.ClusterSchemaInfo;
import org.apache.iotdb.confignode.persistence.NodeInfo;
import org.apache.iotdb.confignode.persistence.PartitionInfo;
+import org.apache.iotdb.confignode.persistence.Snapshot;
import org.apache.iotdb.consensus.common.DataSet;
+import org.apache.thrift.TException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
public class ConfigRequestExecutor {
Review Comment:
OK
--
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] OneSizeFitsQuorum commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867330780
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/Snapshot.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.confignode.persistence;
+
+import org.apache.thrift.TException;
+
+import java.io.File;
+import java.io.IOException;
+
+public interface Snapshot {
Review Comment:
Maybe we could put this interface in the IStateMachine interface of the consensus module, and make a snapshotAPI like the EventAPI, which would be easier to maintain.
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/executor/ConfigRequestExecutor.java:
##########
@@ -44,10 +44,23 @@
import org.apache.iotdb.confignode.persistence.ClusterSchemaInfo;
import org.apache.iotdb.confignode.persistence.NodeInfo;
import org.apache.iotdb.confignode.persistence.PartitionInfo;
+import org.apache.iotdb.confignode.persistence.Snapshot;
import org.apache.iotdb.consensus.common.DataSet;
+import org.apache.thrift.TException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
public class ConfigRequestExecutor {
Review Comment:
should implement Snapshot interface?
--
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] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867318242
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/executor/ConfigRequestExecutor.java:
##########
@@ -138,4 +151,66 @@ public TSStatus executorNonQueryPlan(ConfigRequest req)
throw new UnknownPhysicalPlanTypeException(req.getType());
}
}
+
+ public boolean takeSnapshot(File snapshotDir) {
Review Comment:
`PartitionRegionStateMachine` response snapshot request, and the concrete implementation process are `ConfigRequestExecutor `
--
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] CRZbulabula commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
CRZbulabula commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r866886909
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -330,12 +338,42 @@ public List<TConsensusGroupId> getRegionGroupIds(String storageGroup, TConsensus
return result;
}
- public void serialize(ByteBuffer buffer) {
- // TODO: Serialize ClusterSchemaInfo
+ @Override
+ public boolean takeSnapshot(File snapshotFile) throws IOException {
+
+ File tmpFile = new File(snapshotFile.getAbsolutePath() + "-" + UUID.randomUUID());
+ ByteBuffer buffer = ByteBuffer.allocate(bufferSize);
+
+ storageGroupReadWriteLock.readLock().lock();
+ try {
+ try (FileOutputStream fileOutputStream = new FileOutputStream(tmpFile);
+ FileChannel fileChannel = fileOutputStream.getChannel()) {
+ mTree.serialize(buffer);
+ fileChannel.write(buffer);
+ }
+ return tmpFile.renameTo(snapshotFile);
+ } finally {
+ buffer.clear();
+ tmpFile.delete();
+ storageGroupReadWriteLock.readLock().unlock();
+ }
}
- public void deserialize(ByteBuffer buffer) {
- // TODO: Deserialize ClusterSchemaInfo
+ @Override
+ public void loadSnapshot(File snapshotFile) throws IOException {
Review Comment:
Don't forget to add test for loadSnapshot~
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -330,12 +338,42 @@ public List<TConsensusGroupId> getRegionGroupIds(String storageGroup, TConsensus
return result;
}
- public void serialize(ByteBuffer buffer) {
- // TODO: Serialize ClusterSchemaInfo
+ @Override
+ public boolean takeSnapshot(File snapshotFile) throws IOException {
Review Comment:
Don't forget to add test for takeSnapshot~
--
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] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867318052
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -332,12 +342,59 @@ public List<TConsensusGroupId> getRegionGroupIds(String storageGroup, TConsensus
return result;
}
- public void serialize(ByteBuffer buffer) {
- // TODO: Serialize ClusterSchemaInfo
+ @Override
+ public boolean takeSnapshot(File snapshotDir) throws IOException {
+
+ File snapshotFile = new File(snapshotDir, snapshotFileName);
+ if (snapshotFile.exists() && snapshotFile.isFile()) {
+ LOGGER.error(
+ "Failed to take snapshot, because snapshot file [{}] is already exist.",
+ snapshotFile.getAbsolutePath());
+ return false;
+ }
+
+ File tmpFile = new File(snapshotFile.getAbsolutePath() + "-" + UUID.randomUUID());
+ ByteBuffer buffer = ByteBuffer.allocate(bufferSize);
+
+ storageGroupReadWriteLock.readLock().lock();
Review Comment:
any write request should not be allowed when doing snapshot.
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/executor/ConfigRequestExecutor.java:
##########
@@ -56,6 +67,8 @@ public class ConfigRequestExecutor {
private final AuthorInfo authorInfo;
+ private static final Logger LOGGER = LoggerFactory.getLogger(ConfigRequestExecutor.class);
Review Comment:
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867287481
##########
confignode/src/main/java/org/apache/iotdb/confignode/service/executor/ConfigRequestExecutor.java:
##########
@@ -135,4 +148,56 @@ public TSStatus executorNonQueryPlan(ConfigRequest req)
throw new UnknownPhysicalPlanTypeException(req.getType());
}
}
+
+ public boolean takeSnapshot(File snapshotDir) {
+
+ AtomicBoolean result = new AtomicBoolean(true);
+ getAllAttributes()
+ .parallelStream()
+ .forEach(
+ x -> {
+ boolean takeSnapshotResult = true;
+ try {
+ String fileName = x.getClass().getSimpleName();
Review Comment:
use class name as file name isn't a good idea. If we change the class name, then the old snapshot won't be load success.
--
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] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867590584
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -332,12 +342,59 @@ public List<TConsensusGroupId> getRegionGroupIds(String storageGroup, TConsensus
return result;
}
- public void serialize(ByteBuffer buffer) {
- // TODO: Serialize ClusterSchemaInfo
+ @Override
+ public boolean processTakeSnapshot(File snapshotDir) throws IOException {
+
+ File snapshotFile = new File(snapshotDir, snapshotFileName);
+ if (snapshotFile.exists() && snapshotFile.isFile()) {
+ LOGGER.error(
+ "Failed to take snapshot, because snapshot file [{}] is already exist.",
+ snapshotFile.getAbsolutePath());
+ return false;
+ }
+
+ File tmpFile = new File(snapshotFile.getAbsolutePath() + "-" + UUID.randomUUID());
+ ByteBuffer buffer = ByteBuffer.allocate(bufferSize);
Review Comment:
The serialization and deserialization functions of mtree are provided by the mtree module, which provides interfaces of Bytebuffer type. So I'm doing so for now.
Maybe once we've agreed on the standards and rules for serialization, we can change to a standard way together.
--
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] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867314125
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -330,12 +338,42 @@ public List<TConsensusGroupId> getRegionGroupIds(String storageGroup, TConsensus
return result;
}
- public void serialize(ByteBuffer buffer) {
- // TODO: Serialize ClusterSchemaInfo
+ @Override
+ public boolean takeSnapshot(File snapshotFile) throws IOException {
+
+ File tmpFile = new File(snapshotFile.getAbsolutePath() + "-" + UUID.randomUUID());
+ ByteBuffer buffer = ByteBuffer.allocate(bufferSize);
+
+ storageGroupReadWriteLock.readLock().lock();
+ try {
+ try (FileOutputStream fileOutputStream = new FileOutputStream(tmpFile);
+ FileChannel fileChannel = fileOutputStream.getChannel()) {
+ mTree.serialize(buffer);
+ fileChannel.write(buffer);
+ }
+ return tmpFile.renameTo(snapshotFile);
+ } finally {
+ buffer.clear();
+ tmpFile.delete();
+ storageGroupReadWriteLock.readLock().unlock();
+ }
}
- public void deserialize(ByteBuffer buffer) {
- // TODO: Deserialize ClusterSchemaInfo
+ @Override
+ public void loadSnapshot(File snapshotFile) throws IOException {
Review Comment:
fixed.
##########
confignode/src/main/java/org/apache/iotdb/confignode/service/executor/ConfigRequestExecutor.java:
##########
@@ -135,4 +148,56 @@ public TSStatus executorNonQueryPlan(ConfigRequest req)
throw new UnknownPhysicalPlanTypeException(req.getType());
}
}
+
+ public boolean takeSnapshot(File snapshotDir) {
+
+ AtomicBoolean result = new AtomicBoolean(true);
+ getAllAttributes()
+ .parallelStream()
+ .forEach(
+ x -> {
+ boolean takeSnapshotResult = true;
+ try {
+ String fileName = x.getClass().getSimpleName();
Review Comment:
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867318242
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/executor/ConfigRequestExecutor.java:
##########
@@ -138,4 +151,66 @@ public TSStatus executorNonQueryPlan(ConfigRequest req)
throw new UnknownPhysicalPlanTypeException(req.getType());
}
}
+
+ public boolean takeSnapshot(File snapshotDir) {
Review Comment:
`PartitionRegionStateMachine` response snapshot request, and the concrete implementation process are controlled by `ConfigRequestExecutor `
--
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] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867333781
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/executor/ConfigRequestExecutor.java:
##########
@@ -44,10 +44,23 @@
import org.apache.iotdb.confignode.persistence.ClusterSchemaInfo;
import org.apache.iotdb.confignode.persistence.NodeInfo;
import org.apache.iotdb.confignode.persistence.PartitionInfo;
+import org.apache.iotdb.confignode.persistence.Snapshot;
import org.apache.iotdb.consensus.common.DataSet;
+import org.apache.thrift.TException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
public class ConfigRequestExecutor {
Review Comment:
`ConfigRequestExecutor` should only be an executor with no ability to process snapshots _(It is only responsible for scheduling)_ .
The module that implements the Snapshot interface should be able to operate the snapshot itself. Such as `ClusterSchemaInfo`
--
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] qiaojialin merged pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
qiaojialin merged PR #5816:
URL: https://github.com/apache/iotdb/pull/5816
--
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] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867591144
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -43,23 +43,33 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.UUID;
import java.util.concurrent.locks.ReentrantReadWriteLock;
-public class ClusterSchemaInfo {
+public class ClusterSchemaInfo implements SnapshotProcessor {
private static final Logger LOGGER = LoggerFactory.getLogger(ClusterSchemaInfo.class);
// StorageGroup read write lock
private final ReentrantReadWriteLock storageGroupReadWriteLock;
- // TODO: serialize and deserialize
private MTreeAboveSG mTree;
+ // The size of the buffer used for snapshot(temporary value)
+ private final int bufferSize = 10 * 1024 * 1024;
+
+ private final String snapshotFileName = "ClusterSchemaInfo.st";
Review Comment:
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867334387
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/Snapshot.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.confignode.persistence;
+
+import org.apache.thrift.TException;
+
+import java.io.File;
+import java.io.IOException;
+
+public interface Snapshot {
Review Comment:
We can consider doing so, but this snapshot is where it will actually be processed _(It is not the interface for the consensus group to interact with the state machine)_ . This seems beyond the scope of `IstateMachine`.
Maybe this interface could be moved to `node-commons` ?
--
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] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867314093
##########
confignode/src/main/java/org/apache/iotdb/confignode/service/executor/ConfigRequestExecutor.java:
##########
@@ -135,4 +148,56 @@ public TSStatus executorNonQueryPlan(ConfigRequest req)
throw new UnknownPhysicalPlanTypeException(req.getType());
}
}
+
+ public boolean takeSnapshot(File snapshotDir) {
+
Review Comment:
fixed.
##########
confignode/src/main/java/org/apache/iotdb/confignode/service/executor/ConfigRequestExecutor.java:
##########
@@ -135,4 +148,56 @@ public TSStatus executorNonQueryPlan(ConfigRequest req)
throw new UnknownPhysicalPlanTypeException(req.getType());
}
}
+
+ public boolean takeSnapshot(File snapshotDir) {
+
+ AtomicBoolean result = new AtomicBoolean(true);
+ getAllAttributes()
+ .parallelStream()
+ .forEach(
+ x -> {
+ boolean takeSnapshotResult = true;
+ try {
+ String fileName = x.getClass().getSimpleName();
+ File file = new File(snapshotDir, fileName);
+ takeSnapshotResult = x.takeSnapshot(file);
+ } catch (TException | IOException e) {
+ LOGGER.error(e.getMessage());
+ takeSnapshotResult = false;
+ } finally {
+ // If any snapshot fails, the whole fails
+ // So this is just going to be false
+ if (!takeSnapshotResult) {
+ result.set(false);
+ }
+ }
+ });
+ return result.get();
+ }
+
+ public void loadSnapshot(File latestSnapshotRootDir) {
+
Review Comment:
fixed.
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -330,12 +338,42 @@ public List<TConsensusGroupId> getRegionGroupIds(String storageGroup, TConsensus
return result;
}
- public void serialize(ByteBuffer buffer) {
- // TODO: Serialize ClusterSchemaInfo
+ @Override
+ public boolean takeSnapshot(File snapshotFile) throws IOException {
Review Comment:
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867334387
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/Snapshot.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.confignode.persistence;
+
+import org.apache.thrift.TException;
+
+import java.io.File;
+import java.io.IOException;
+
+public interface Snapshot {
Review Comment:
We can consider doing so, but this snapshot is where it will actually be processed _(It is not the interface for the consensus group to interact with the state machine)_ . This seems beyond the scope of `IstateMachine`.
--
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] cigarl commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
cigarl commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867338767
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/Snapshot.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.confignode.persistence;
+
+import org.apache.thrift.TException;
+
+import java.io.File;
+import java.io.IOException;
+
+public interface Snapshot {
Review Comment:
already updated
--
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] qiaojialin commented on a diff in pull request #5816: [IOTDB-3098] ClusterSchemaInfo snapshot interface
Posted by GitBox <gi...@apache.org>.
qiaojialin commented on code in PR #5816:
URL: https://github.com/apache/iotdb/pull/5816#discussion_r867488422
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -332,12 +342,59 @@ public List<TConsensusGroupId> getRegionGroupIds(String storageGroup, TConsensus
return result;
}
- public void serialize(ByteBuffer buffer) {
- // TODO: Serialize ClusterSchemaInfo
+ @Override
+ public boolean processTakeSnapshot(File snapshotDir) throws IOException {
+
+ File snapshotFile = new File(snapshotDir, snapshotFileName);
+ if (snapshotFile.exists() && snapshotFile.isFile()) {
+ LOGGER.error(
+ "Failed to take snapshot, because snapshot file [{}] is already exist.",
+ snapshotFile.getAbsolutePath());
+ return false;
+ }
+
+ File tmpFile = new File(snapshotFile.getAbsolutePath() + "-" + UUID.randomUUID());
+ ByteBuffer buffer = ByteBuffer.allocate(bufferSize);
Review Comment:
How about using ByteArrayOutputStream first?
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/ClusterSchemaInfo.java:
##########
@@ -43,23 +43,33 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.UUID;
import java.util.concurrent.locks.ReentrantReadWriteLock;
-public class ClusterSchemaInfo {
+public class ClusterSchemaInfo implements SnapshotProcessor {
private static final Logger LOGGER = LoggerFactory.getLogger(ClusterSchemaInfo.class);
// StorageGroup read write lock
private final ReentrantReadWriteLock storageGroupReadWriteLock;
- // TODO: serialize and deserialize
private MTreeAboveSG mTree;
+ // The size of the buffer used for snapshot(temporary value)
+ private final int bufferSize = 10 * 1024 * 1024;
+
+ private final String snapshotFileName = "ClusterSchemaInfo.st";
Review Comment:
+1
--
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