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: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
