Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QPID-7972: [Broker-J] Put VirtualHostNode in ERRORed state if the underlying VirtualHost is #4

Closed
wants to merge 1 commit into from

Conversation

overmeulen
Copy link

No description provided.

Copy link

@adel-boutros adel-boutros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Olivier,

It seems you are mixing in your pull request 2 different issues: One related to the Error state of the virtualHost and the other one about problems with different locales (I assume tests are failing on your machine because it is configured with the FR locale).

In my opinion, it is better to split those into 2 different pull requests to have a better context

@k-wall
Copy link
Contributor

k-wall commented Feb 22, 2018

Hello Olivier

Thanks for the contribution. It is much appreciated.

I am not too uncomfortable with the idea of special casing of the state behaviour for this one type of VirtualHostNode.

I think in the long term, in general parent objects should have some way to react to the state of the their child, but I think this would want to be done within the common mechanics of AbstractConfiguredObject rather than applied piecemeal. It would be useful to be able to apply different rules - for instance a virtualhost might be considered critical whereas a BrokerSyslogLogger not. This would be a larger piece of work.

I am looking for another way to solve your problem. Looking at BrokerImpl#performActivation, it currently checks the immediate decedents of the Broker if broker.failStartupWithErroredChild is set. We could alter this code so it can either check immediate descendants or the state of the entire tree. If the code had that ability would your use case by answered?

@overmeulen
Copy link
Author

Hello,

Yes it would answer my use case. Actually I considered this option when doing the dev but I wasn't very fond of the Broker checking the whole tree of children nodes... I found it better to keep each node aware of just its direct children and it looked more correct to actually put the VirtualHostNode in ERRORed state too.

Olivier

@asfgit asfgit closed this in b720d7d Feb 23, 2018
asfgit pushed a commit that referenced this pull request Feb 26, 2018
…nfigured object hierarchy

This closes #4
#4

(cherry picked from commit b720d7d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants