Uh oh!
There was an error while loading. Please reload this page.
CLOUDSTACK-8855 Improve Error Message for Host Alert State and reconnect host API. - #2380
CLOUDSTACK-8855 Improve Error Message for Host Alert State and reconnect host API.#2380DaanHoogland wants to merge 5 commits into
Conversation
DaanHoogland
commented
Dec 30, 2017
@rafaelweingartner I can't add you as a reviewer. Are you member of the apache organisation? |
yadvr
commented
Jan 2, 2018
@DaanHoogland can you fix the build failures? |
rafaelweingartner
commented
Jan 2, 2018
@DaanHoogland that is probably because I am not in the Gitbox experiment. I would say that here I see the same problem as in #837. It feels so simple to create a new column in "AlertVO" table to add the content. I am not going to repeat the discussion we had there here. So, as long as others are ok with it. With the current state of the PR I give a -0. |
DaanHoogland
commented
Jan 3, 2018
@rafaelweingartner@marcaurele @rhtyd I am not picking this up. I just 're-opened' this because there is no movement. Please send me your PR if you want to see anything changed. |
rafaelweingartner
commented
Jan 3, 2018
@DaanHoogland ok... |
rafaelweingartner
commented
Jan 4, 2018
@DaanHoogland I tried to push these changes to your branch, but it seems that I do not have permissions to do so. Shall I open a PR for your branch? |
DaanHoogland
commented
Jan 4, 2018
Hey @rafaelweingartner If I new how i would a few seconds ago have given you access, you know that ;) |
rafaelweingartner
commented
Jan 4, 2018
@DaanHoogland PR created. |
| if (host.getStatus() == Status.Disconnected) { | ||
| s_logger.info("Host is already disconnected, no work to be done"); | ||
| return true; | ||
| throw new CloudRuntimeException("Host is already disconnected, no work to be done"); |
There was a problem hiding this comment.
This looks wrong to me, why throwing an exception when nothing has to be done?
There was a problem hiding this comment.
I agree on this one. And besides that, if we take a closer look, the guy that proposed this change, was replacing the return of boolean values with exceptions to indicate some error. It makes sense that a false return would generate an exception while trying to reconnect as the case at line 1002. This one was returning true, so it feels that we should not throw an exception here.
| try { | ||
| _agentMgr.reconnect(host.getId()); | ||
| } catch (Exception e ) { | ||
| s_logger.info("failed to reconnect host "+host); |
rafaelweingartner
commented
Jan 4, 2018
@marcaurele thanks for your feedback here. I already created a PR to Daan's branch. |
I also removed some unused variables and comments refering Alex... Who is Alex?
| if (host.getStatus() == Status.Disconnected) { | ||
| s_logger.info("Host is already disconnected, no work to be done"); | ||
| return true; | ||
| s_logger.debug("Host is already disconnected, no work to be done: " + hostId); |
There was a problem hiding this comment.
@DaanHoogland I made a mistake here. Can you fix it for me?
You need to add a return;, right after line 994
| public void pullAgentOutMaintenance(long hostId); | ||
| boolean reconnect(long hostId); | ||
| void reconnect(long hostId) throws AgentUnavailableException; |
There was a problem hiding this comment.
this change is breaking things. We need to look at it. @rafaelweingartner CAManagerImpl.java needs the boolean. should we take this to 4.12? I'm not taking the time now.
There was a problem hiding this comment.
Hmm.. Let me try to look into it today...
There was a problem hiding this comment.
Is the problem during compilation or execution?
There was a problem hiding this comment.
Never mind. Found the problem. I will work on it.
There was a problem hiding this comment.
I fixed the problem and opened a PR to send this whole contribution. This makes things easier for me.
PR: #2387
this closes#837