adoroszlai commented on a change in pull request #2953:
URL: https://github.com/apache/ozone/pull/2953#discussion_r807614019
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java
##########
@@ -387,10 +387,20 @@ public void reinitialize() throws IOException {
@Override
public void close() throws IOException {
+ super.close();
+ stop();
+ try {
+ ExitUtils.terminate(1, "scm state machine terminated by ratis", LOG);
+ } catch (ExitUtils.ExitException e) {
+ LOG.info("scm state machine is terminated by Ratis, " +
+ "but termination is disabled: {}", e.getMessage());
+ }
Review comment:
If you don't want to propagate the exception, there is a parameter for
`terminate` to skip throwing it.
```suggestion
ExitUtils.terminate(1, "scm state machine terminated by ratis",
null, false, LOG);
```
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java
##########
@@ -470,8 +469,17 @@ public void close() throws IOException {
// OM should be shutdown as the StateMachine has shutdown.
LOG.info("StateMachine has shutdown. Shutdown OzoneManager if not " +
"already shutdown.");
- ozoneManager.shutdown(new RaftException("RaftServer called shutdown on " +
- "StateMachine"));
+ super.close();
+ if (!ozoneManager.isOmStopped()) {
+ ozoneManager.stop();
+ }
+ try {
+ ExitUtils.terminate(1,
+ "OM state machine is shutdown by Ratis server", LOG);
+ } catch (ExitUtils.ExitException e) {
+ LOG.info("om state machine is terminated by Ratis, " +
+ "but termination is disabled: {}", e.getMessage());
+ }
Review comment:
Same comment here about catching the exception:
```suggestion
ExitUtils.terminate(1, "OM state machine is shutdown by Ratis server",
null, false, LOG);
```
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java
##########
@@ -470,8 +469,17 @@ public void close() throws IOException {
// OM should be shutdown as the StateMachine has shutdown.
LOG.info("StateMachine has shutdown. Shutdown OzoneManager if not " +
"already shutdown.");
- ozoneManager.shutdown(new RaftException("RaftServer called shutdown on " +
- "StateMachine"));
+ super.close();
+ if (!ozoneManager.isOmStopped()) {
Review comment:
Nit:
```suggestion
if (!ozoneManager.isStopped()) {
```
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java
##########
@@ -387,10 +387,20 @@ public void reinitialize() throws IOException {
@Override
public void close() throws IOException {
+ super.close();
+ stop();
+ try {
+ ExitUtils.terminate(1, "scm state machine terminated by ratis", LOG);
+ } catch (ExitUtils.ExitException e) {
+ LOG.info("scm state machine is terminated by Ratis, " +
+ "but termination is disabled: {}", e.getMessage());
+ }
+ }
+
+ public void stop() throws IOException {
if (!isInitialized) {
return;
}
Review comment:
Should we keep this check in `close`, to avoid exit if not initialized?
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -560,6 +560,10 @@ private OzoneManager(OzoneConfiguration conf,
StartupOption startupOption)
}
}
+ public boolean isOmStopped() {
Review comment:
Nit:
```suggestion
public boolean isStopped() {
```
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1028,6 +1029,7 @@ private static void
initializeSecurityIfNeeded(OzoneConfiguration conf,
*/
public static boolean scmInit(OzoneConfiguration conf,
String clusterId) throws IOException {
+ ExitUtils.disableSystemExit();
Review comment:
Why is `disableSystemExit` needed in production code? (I guess this has
to do with `stop` vs. `close` in state machine.)
--
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]
