junkaixue commented on a change in pull request #1812:
URL: https://github.com/apache/helix/pull/1812#discussion_r667391166
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -339,21 +361,24 @@ private void createLiveInstance() {
* carry over current-states from last sessions
* set to initial state for current session only when state doesn't exist in
current session
*/
- private void carryOverPreviousCurrentState() {
- List<String> sessions =
_dataAccessor.getChildNames(_keyBuilder.sessions(_instanceName));
+ public static synchronized void
carryOverPreviousCurrentState(HelixDataAccessor dataAccessor,
+ PropertyKey.Builder keyBuilder, String instanceName, String sessionId,
Review comment:
You can remove the keyBuilder here. Because it can be fetched as:
accessor.keyBuilder().
So there are several places you can simplify the code.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1376,12 +1384,27 @@ public void handleNewSession(String sessionId) throws
Exception {
}
}
- void handleNewSessionAsParticipant(final String sessionId) throws Exception {
+ private void handleNewSessionInManagementMode(String sessionId) throws
Exception {
+ LOG.info("Skip reset because instance is in {} status",
LiveInstance.LiveInstanceStatus.PAUSED);
+ if (!InstanceType.PARTICIPANT.equals(_instanceType)) {
Review comment:
You did not get my idea. What I am saying is in the line 1324, you only
checks for participant. And do not check for CONTROLLER_PARTICIPANT. Do you
mean controller participant will never apply for management mode? This is not
consistent with participant.
Also this logic is too specific and makes the logic complicated... We need a
better way to handle it.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -368,21 +393,35 @@ private void carryOverPreviousCurrentState() {
continue;
}
- StateModelDefinition stateModel =
-
_dataAccessor.getProperty(_keyBuilder.stateModelDef(stateModelDefRef));
+ StateModelDefinition stateModelDef =
+
dataAccessor.getProperty(keyBuilder.stateModelDef(stateModelDefRef));
+ String initState = stateModelDef.getInitialState();
+ Map<String, String> partitionExpectedStateMap = new HashMap<>();
+ if (setToInitState) {
+ lastCurState.getPartitionStateMap().keySet()
+ .forEach(partition -> partitionExpectedStateMap.put(partition,
initState));
+ } else {
+ String factoryName = lastCurState.getStateModelFactoryName();
+ StateModelFactory<? extends StateModel> stateModelFactory =
+ stateMachineEngine.getStateModelFactory(stateModelDefRef,
factoryName);
+ lastCurState.getPartitionStateMap().keySet().forEach(partition -> {
+ StateModel stateModel =
+
stateModelFactory.getStateModel(lastCurState.getResourceName(), partition);
+ partitionExpectedStateMap.put(partition,
stateModel.getCurrentState());
Review comment:
NIT: let's simplify the code there. stateMode is not used in following
code. Then it could be:
partitionExpectedStateMap.put(partition,
stateModelFactory.getStateModel(lastCurState.getResourceName(),
partition).getCurrentState());
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -413,13 +452,16 @@ private void carryOverPreviousCurrentState() {
* remove previous current state parent nodes
*/
for (String session : sessions) {
- if (session.equals(_sessionId)) {
+ if (session.equals(sessionId)) {
continue;
}
- String path = _keyBuilder.currentStates(_instanceName,
session).getPath();
- LOG.info("Removing current states from previous sessions. path: " +
path);
- _zkclient.deleteRecursively(path);
+ PropertyKey currentStatesProperty =
keyBuilder.currentStates(instanceName, session);
+ String path = currentStatesProperty.getPath();
+ LOG.info("Removing current states from previous sessions. path: {}",
path);
+ if (!dataAccessor.removeProperty(currentStatesProperty)) {
+ throw new ZkClientException("Failed to delete " + path);
Review comment:
Let's not throw exception here. We should have an error message. Failed
to delete the old session info can leave some stale data but will not impact
the correctness. If we stop here, then the participant reconnect will be failed.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CurStateCarryOverUpdater.java
##########
@@ -35,16 +35,17 @@
*/
class CurStateCarryOverUpdater implements DataUpdater<ZNRecord> {
final String _curSessionId;
- final String _initState;
+ final Map<String, String> _initStateMap;
Review comment:
You pass in the expected state map. But here you still name it as init
state map, it could be confusing. Let's change the 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
