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/04/01 11:28:09 UTC
[GitHub] [hadoop-ozone] elek opened a new pull request #745: HDDS-3315. Use
EventQueue for delayed/immediate safe mode notifications
elek opened a new pull request #745: HDDS-3315. Use EventQueue for delayed/immediate safe mode notifications
URL: https://github.com/apache/hadoop-ozone/pull/745
SCM is built from loosely coupled components which communicate with async event with each other.
Using the same abstraction (EventQueue) has the benefit that we can use the same visibility / testing tools such as the 'ozone insight' definition (which makes visible all the messages) or the test handler (which can wait until all the event queue messages are processed)
During the review of HDDS-3221 it was suggested (by me) to use the EventQueue instead of the new SafeModeNotification interface.
There was only one counter argument against it:
> 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
I respect this opinion, but I think it's better to use one abstraction and a consistent architecture inside one component (together with all the existing limitations). 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.
In this patch I would like to show how the previous listener interface can be replaced by the EventQueue.
It (hopefully) shows that this is not complex, and in fact can help us to decouple different component from each other
## What changes were proposed in this pull request?
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3315
## How was this patch tested?
With the existing unit test.
----------------------------------------------------------------
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 #745: HDDS-3315. Use
EventQueue for delayed/immediate safe mode notifications
Posted by GitBox <gi...@apache.org>.
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-613921286
@sodonnel Thanks your offer. I rebased it and it seems to be fine now (the failure is an independent problem, we investigate it on master).
But I would appreciate any help with the review.
----------------------------------------------------------------
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 #745: HDDS-3315. Use
EventQueue for delayed/immediate safe mode notifications
Posted by 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-613954771
This change LGTM. My only comment, which is nice to have, is that I wonder if we should move the logic in `SafeModeHandler.onMessage()` into `SCMSafeModeManager.emitSafeModeStatus()` - that would allow us to get rid of that class entirely and make the logic slightly easier to follow.
However I am +1 on this how it is, so its up to you if you think it makes sense to make that further 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] sodonnel commented on issue #745: HDDS-3315. Use
EventQueue for delayed/immediate safe mode notifications
Posted by 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-613348383
@elek Do you want me to take this one over, or do you want to resolve the conflicts? I am happy with either choice.
----------------------------------------------------------------
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 #745: HDDS-3315. Use
EventQueue for delayed/immediate safe mode notifications
Posted by GitBox <gi...@apache.org>.
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:
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 #745: HDDS-3315. Use
EventQueue for delayed/immediate safe mode notifications
Posted by 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-615271955
Followup changes LGTM. +1 on this after CI comes back green.
----------------------------------------------------------------
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 #745: HDDS-3315. Use
EventQueue for delayed/immediate safe mode notifications
Posted by GitBox <gi...@apache.org>.
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-615224653
Thanks the review @sodonnel
> I wonder if we should move the logic in SafeModeHandler.onMessage() into SCMSafeModeManager.emitSafeModeStatus() - that would allow us to get rid of that class entirely and make the logic slightly easier to follow.
Good idea, and it seems to be a small change. I added it to this patch (+ I improved to unit test a little bit during the move).
----------------------------------------------------------------
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 #745: HDDS-3315. Use
EventQueue for delayed/immediate safe mode notifications
Posted by 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