elek commented on issue #745: HDDS-3315. Use EventQueue for delayed/immediate safe mode notifications URL: https://github.com/apache/hadoop-ozone/pull/745#issuecomment-607664021 > This results in code which is hard to understand for a newcomer and impossible to follow the flow of things in an IDE. It's hard to judge. But the newcomer should understand `EventQueue` any way as this is the heart of the SCM. My proposal is to use a **consistent** development style. Currently this is `EventQueue`. We can switch to an other approach but in that case, we should replace `EventQueue` everywhere. > then the objects are notified in a clear and decoupled way using a simple interface It can be subjective, but it's not decoupled. `EventQueue` is decoupled. But in the current code `SafeModeHandler` should have references to other objects. With `EventQueue` you don't need to have references: --> more decoupled, easier to test. > 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. Agree. I think safe mode handler is not needed as the functionality can be replaced by the existing functionality of `EventQueue`. As you see after this patch it's used only for delayed notification. I would check if this delayed notification is required or not. > 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 That was the original implementation but during the reviews it was suggested by others to collect all the flow registration to one single place TLDR, my arguments to use EventQueue here: * consistent development style (same structure is used everywhere). * increased decoupling (easier to test, no direct references to other classes) * increased visibility (we have existing metrics / debug tools to check EventQueue state messages) As far as I understood the counter argument is that it might be harder to follow to call hierarchy. It might be true, but I think the previous arguments are more important (compared to make SCM more easy to understand for newcomers...)
---------------------------------------------------------------- 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]
