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/10/10 15:37:55 UTC
[GitHub] [iotdb] SpriCoder commented on a diff in pull request #7531: [IOTDB-4550] Add NodeId to Peer
SpriCoder commented on code in PR #7531:
URL: https://github.com/apache/iotdb/pull/7531#discussion_r991408543
##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java:
##########
@@ -202,9 +205,12 @@ public ConfigManager() throws IOException {
this.loadManager = new LoadManager(this);
this.syncManager = new SyncManager(this, syncInfo);
- // ConsensusManager must be initialized last, as it would load states from disk and reinitialize
- // above managers
- this.consensusManager = new ConsensusManager(this, stateMachine);
+ // ConsensusManager doesn't initialize until the ConfigNode knows its own nodeId
+ this.consensusManager = null;
+ }
+
+ public void initConsensusManager() throws IOException {
+ this.consensusManager = new ConsensusManager(this, this.stateMachine);
Review Comment:
Please add check of whether consensusManager is already initialized. if this method is called be a Thread-Safe method, then you can simply judge whether consensusManager is null. Or else you need to guarantee consensusManager only be initialized one time.
##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/IManager.java:
##########
@@ -265,9 +266,9 @@ TDataPartitionTableResp getOrCreateDataPartition(
/**
* Register ConfigNode when it is first startup
*
- * @return TSStatus
+ * @return TConfigNodeRegisterResp
*/
Review Comment:
From my perspective, I think if there are no explain of param, there are no need to mention in java doc.
##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java:
##########
@@ -727,8 +736,18 @@ private TSStatus checkConfigNodeGlobalConfig(TConfigNodeRegisterReq req) {
@Override
public TSStatus createPeerForConsensusGroup(List<TConfigNodeLocation> configNodeLocations) {
- consensusManager.createPeerForConsensusGroup(configNodeLocations);
- return StatusUtils.OK;
+ for (int i = 0; i < 30; i++) {
+ try {
+ if (consensusManager == null) {
+ Thread.sleep(1000);
+ } else {
+ consensusManager.createPeerForConsensusGroup(configNodeLocations);
+ return StatusUtils.OK;
+ }
+ } catch (Exception ignored) {
+ }
Review Comment:
This process of Exception is too simply. From my perspective, this way may ignore some useful exception caused by createPeerForConsensusGroup, so I think you can use multi catch in level.
##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java:
##########
@@ -657,21 +663,24 @@ public TPermissionInfoResp checkUserPrivileges(
}
@Override
- public TSStatus registerConfigNode(TConfigNodeRegisterReq req) {
+ public TConfigNodeRegisterResp registerConfigNode(TConfigNodeRegisterReq req) {
// Check global configuration
TSStatus status = confirmLeader();
if (status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
TSStatus errorStatus = checkConfigNodeGlobalConfig(req);
if (errorStatus != null) {
- return errorStatus;
+ return new TConfigNodeRegisterResp().setStatus(errorStatus).setConfigNodeId(-1);
Review Comment:
Please Avoid Magic Number in Code. You use static final param.
##########
confignode/src/main/java/org/apache/iotdb/confignode/service/ConfigNode.java:
##########
@@ -90,7 +91,10 @@ public void active() {
/* Initial startup of Seed-ConfigNode */
if (ConfigNodeDescriptor.getInstance().isSeedConfigNode()) {
+ configManager.initConsensusManager();
+
SystemPropertiesUtils.storeSystemParameters();
+ SystemPropertiesUtils.storeConfigNodeId(0);
Review Comment:
Also, Please avoid magic number.
##########
confignode/src/main/java/org/apache/iotdb/confignode/conf/SystemPropertiesUtils.java:
##########
@@ -239,6 +239,16 @@ public static void storeConfigNodeList(List<TConfigNodeLocation> configNodes) th
storeSystemProperties(systemProperties);
}
+ public static void storeConfigNodeId(int nodeId) throws IOException {
+ if (!systemPropertiesFile.exists()) {
+ return;
+ }
Review Comment:
Maybe there can add some error logs.
##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java:
##########
@@ -657,21 +663,24 @@ public TPermissionInfoResp checkUserPrivileges(
}
@Override
- public TSStatus registerConfigNode(TConfigNodeRegisterReq req) {
+ public TConfigNodeRegisterResp registerConfigNode(TConfigNodeRegisterReq req) {
// Check global configuration
TSStatus status = confirmLeader();
if (status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
TSStatus errorStatus = checkConfigNodeGlobalConfig(req);
if (errorStatus != null) {
- return errorStatus;
+ return new TConfigNodeRegisterResp().setStatus(errorStatus).setConfigNodeId(-1);
}
+ int nodeId = nodeManager.generateNodeId();
+ req.getConfigNodeLocation().setConfigNodeId(nodeId);
+
procedureManager.addConfigNode(req);
- return StatusUtils.OK;
+ return new TConfigNodeRegisterResp().setStatus(StatusUtils.OK).setConfigNodeId(nodeId);
}
- return status;
+ return new TConfigNodeRegisterResp().setStatus(status).setConfigNodeId(-1);
Review Comment:
Avoid Magic Number Please.
##########
confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/PartitionRegionStateMachine.java:
##########
@@ -145,16 +147,18 @@ protected DataSet read(ConfigPhysicalPlan plan) {
}
@Override
- public void notifyLeaderChanged(ConsensusGroupId groupId, TEndPoint newLeader) {
- if (currentNode.equals(newLeader)) {
+ public void notifyLeaderChanged(ConsensusGroupId groupId, int newLeader) {
+ if (currentNodeId == newLeader) {
LOGGER.info("Current node {} becomes Leader", newLeader);
configManager.getProcedureManager().shiftExecutor(true);
configManager.getLoadManager().startLoadBalancingService();
configManager.getNodeManager().startHeartbeatService();
configManager.getPartitionManager().startRegionCleaner();
} else {
LOGGER.info(
- "Current node {} is not longer the leader, the new leader is {}", currentNode, newLeader);
+ "Current node {} is not longer the leader, the new leader is {}",
+ currentNodeId,
+ newLeader);
Review Comment:
Support~. If there are only index, it's hard to debug
--
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