[GitHub] [hadoop-ozone] sodonnel commented on issue #745: HDDS-3315. Use EventQueue for delayed/immediate safe mode notifications

[GitBox <gi...@apache.org>]

sodonnel commented on issue #745: HDDS-3315. Use EventQueue for delayed/immediate safe mode notifications
URL: https://github.com/apache/hadoop-ozone/pull/745#issuecomment-607323990
 
 
   I stand by my earlier comment that this style of code is much harder to follow. Events are registered far away from the code which uses them and fired from far away too. This results in code which is hard to understand for a newcomer and impossible to follow the flow of things in an IDE.
   
   The way things are right now, the safe mode stuff is fairly self contained. Events are handled by SafemodeManager, safemode changes are dispatched to SafeModeHandler via events and then the objects are notified in a clear and decoupled way using a simple interface. It could be argued that even the SafemodeHandler class is not be needed, as SafemodeManager is validating the rules, just to dispatch an event to another class, for it to notify the remaining objects.
   
   This change just adds more indirection for marginal and subjective benefit.
   
   Maybe the eventQueue would be easier to follow if the classes that needed to listen for events, registered them at their own point of construction (within their constructors or factories), as then it would be more obvious to someone reading the code, but that may have other problems (eg passing the queue to the constructors).
   
   > The EventQueue is not the only one possible solution, but an existing one. We can either design and switch to a new one or use the existing one.
   
   This does not have to be all or nothing. It is perfectly valid for the SafemodeHandler (which uses event queue) to notify a list of objects directly rather than raising further events to do it. After the changes in HDDS-3221 this interface is much clearer and the coupling with the objects has been reduced. I feel it meets the goal well.

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