You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/03/16 23:21:33 UTC

[GitHub] [hadoop-ozone] sodonnel opened a new pull request #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

sodonnel opened a new pull request #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690

## What changes were proposed in this pull request?

The SafeModeHandler currently accepts several objects which it notifies when the safe mode status changes.

Each of these object are notified using a different method (there is no "notification interface") and some of the logic which really belongs in those objects (ie what to do when safemode goes on or off) is in the safemode classes rather than in the receiving class.

As we may need to extend safemode somewhat to delay pipeline creation until sufficient nodes have registered, I think it is worthwhile to refactor this area to do the following:

1. Introduce a new Interface "SafeModeTransition" which must be implemented by any object which wants to listen for safemode starting or ending.
```
public interface SafeModeTransition {
void handleSafeModeTransition(SCMSafeModeManager.SafeModeStatus status);
}
```
2. Pass the SafeModeStatus object over this new interface. That way, we can extend SafeModeStatus to include more states in the future than just safemode = true / false.

3. Change the constructor of SafeModeHandler to allow any number of objects to be registered to make it more flexible going forward.

4. Ensure the logic of what action to take on safemode transition lives within the notified objects rather than in the Safemode clases.

## What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3221

## How was this patch tested?

Depends on existing unit tests

----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

bharatviswa504 commented on a change in pull request #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#discussion_r393379759

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/safemode/SafeModeTransition.java
##########
@@ -0,0 +1,25 @@
+/*
+ * 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
+ * <p>
+ * <p>http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * <p>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.hadoop.hdds.scm.safemode;
+
+/**
+ * Interface which should be implemented by any object that wishes to be
+ * notified by the SafeModeManager when the safe mode state changes.
+ */
+public interface SafeModeTransition {

Review comment:
NIT: Can we rename this to as SafeModeNotification. As the classes which need to receive this notification will implement them. Instead of specifying Transition in the name of the interface. Thoughts?

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [hadoop-ozone] sodonnel commented on a change in pull request #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

sodonnel commented on a change in pull request #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#discussion_r393485617

Review comment:
Yea, I can make that change. Notification probably sounds better than transition.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [hadoop-ozone] bharatviswa504 commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

bharatviswa504 commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#issuecomment-600851742
 
 
   > Thanks @sodonnel for working on this.
   > 
   > I feel that instead of maintaining safemode state at different places, it should be maintained in one single place and that value should be accessed where ever required.
   > 
   > Safemode state change event handler should only worry about starting the services that are required to be started while coming out of safemode.
   > 
   > I agree that the existing code doesn't reflect this, I have always wanted to make this change.
   
   For cases like clientProtocolServer, blockManager we have a check in safemode not allow few operations. So, these cannot be handled by safemodehandler. Otherwise I need to pass safemodehandler object to those classes. I am talking about the current code.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] elek commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

elek commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#issuecomment-601129563

Thanks the answer. I understand that EventQueue seems to be more complex but as it's the heart of the SCM I would prefer to make it more transparent if this is the problem. I think an async event handler can decrease the coupling instead of using strong coupling which makes the testing more simple and reliable (even if it seems to be a little bit more complex...)

> > That said, please let me know if you have a strong preference to move to the EventQueue model.

I don't understand fully your answer. Do you propose to start to use EventQueue in a separated patch but before the other refactors, or do you propose to skip the usage of EventQueue until the far future (= don't use it).

I think it would be useful to use just one unified notification interface inside SCM>

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [hadoop-ozone] elek commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

elek commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#issuecomment-600737932
 
 
   Thanks @sodonnel , agree the new code looks better.
   
   One question: Just wondering if the `EventQueue` can be used instead of an interface. It would provide additional functions (easy to debug with `ozone insight`, metrics, ...)
   
   What do you think?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] nandakumar131 commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

nandakumar131 commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#issuecomment-600814923
 
 
   Thanks @sodonnel for working on this.
   
   I feel that instead of maintaining safemode state at different places, it should be maintained in one single place and that value should be accessed where ever required.
   
   Safemode state change event handler should only worry about starting the services that are required to be started while coming out of safemode.
   
   I agree that the existing code doesn't reflect this, I have always wanted to make this change.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] bharatviswa504 edited a comment on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

bharatviswa504 edited a comment on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#issuecomment-600851742
 
 
   > Thanks @sodonnel for working on this.
   > 
   > I feel that instead of maintaining safemode state at different places, it should be maintained in one single place and that value should be accessed where ever required.
   > 
   > Safemode state change event handler should only worry about starting the services that are required to be started while coming out of safemode.
   > 
   > I agree that the existing code doesn't reflect this, I have always wanted to make this change.
   
   For cases like clientProtocolServer, blockManager we have a check in safemode and not allow few operations inside those classes. So, these cannot be handled by safemodehandler. Otherwise I need to pass safemodehandler object to those classes. I am talking about the current code.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] sodonnel commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

sodonnel commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#issuecomment-601154884
 
 
   > I don't understand fully your answer. Do you propose to start to use EventQueue in a separated patch but before the other refactors, or do you propose to skip the usage of EventQueue until the far future (= don't use it).
   
   My proposal is to commit this patch, then introduce logic to split the safemode rules into stage-1 / stage-2 to solve the problems we have seen with Network topology. Once that is done, we can see if it makes sense to move to the event queue model. I have a few other higher priority things to complete first, so it likely won't happen in the near future.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] sodonnel commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

sodonnel commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#issuecomment-601637938
 
 
   Bahart has given this a +1. I spoke to Nanda offline who is also happy.
   
   I am going to commit this by end of day today unless anyone objects and open a follow up Jira to investigate moving to the eventQueue model in the future.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] sodonnel commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

sodonnel commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#issuecomment-600895440
 
 
   > Safemode state change event handler should only worry about starting the services that are required to be started while coming out of safemode.
   
   I see the safemode handlers job as dispatching a notification to which ever objects are interested on safemode state change and allow the receiving objects to decide what to do with that notification. We could flip it around and have the other objects poll safemode handler but that does not feel as generic to me.
   
   > I agree that the existing code doesn't reflect this, I have always wanted to make this change.
   
   Are you saying that the changes I have made move things in the direction you would like to see, and are therefore happy with this change?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

bharatviswa504 commented on a change in pull request #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#discussion_r393379759
 
 

 ##########
 File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/safemode/SafeModeTransition.java
 ##########
 @@ -0,0 +1,25 @@
+/*
+ * 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
+ * <p>
+ * <p>http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * <p>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.hadoop.hdds.scm.safemode;
+
+/**
+ * Interface which should be implemented by any object that wishes to be
+ * notified by the SafeModeManager when the safe mode state changes.
+ */
+public interface SafeModeTransition {
 
 Review comment:
   NIT: Can we rename this to as SafeModeNotification

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] sodonnel commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

sodonnel commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#issuecomment-600762562
 
 
   > Thanks @sodonnel , agree the new code looks better.
   > 
   > One question: Just wondering if the `EventQueue` can be used instead of an interface. It would provide additional functions (easy to debug with `ozone insight`, metrics, ...)
   > 
   > What do you think?
   
   I guess it depends on your preference. I personally find the event queue logic hard to follow due to its async nature (you cannot just follow method calls in the IDE). Its not bad, but more difficult when you don't yet understand it, while registering some instances to be notified is easy to follow in an IDE. This is of course a subjective opinion :)
   
   The safe mode logic already makes use of events, so it would certainly work. Infact the SafeModeHandler is invoked by an event. However it would take me more time to change it all to be this way, and tests would need to be reworked too I think (have not checked it detail).
   
   The purpose of this change is not just to clean up the code, but to get it into shape where we can implement a "two stage" safe mode process so we can delay pipeline creation until some of the safemode rules have passed (for network topology). I think it would be more useful to continue with this change as it is, get the "two stage" part working and then consider a further refactor later if it still makes sense.
   
   That said, please let me know if you have a strong preference to move to the EventQueue model.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] sodonnel merged pull request #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

sodonnel merged pull request #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] elek commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface

Posted by GitBox <gi...@apache.org>.

elek commented on issue #690: HDDS-3221. Refactor SafeModeHandler to use a Notification Interface
URL: https://github.com/apache/hadoop-ozone/pull/690#issuecomment-604894837
 
 
   I am not full happy that my suggestion is ignored. 
   
   I think it would be better to switch to the EventQueue unless we have a strong arguments against it (what I would accept happily, but I haven't see it yet).
   
   I don't think it's a good idea to develop more complexity on top of a structure what we would like to replace very soon... 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org