Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98589348
I think there is agreement that this is a partial fix (with the right
intent). And that this PR doesn't add any value from the end-users perspective.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98592118
This does add value to us. It is used to page engineers on duty.
Op ma 4 mei 2015 om 07:06 schreef koushik-das notificati...@github.com:
I
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98471048
@abhinandanprateek Nice write up! +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98476949
@bhaisaab I've built 4.5.1 from your branch
HA-abstractinvestigatorimpl-nullstate, built everything from scratch. When I
shut down all nodes in a cluster, they
Github user bhaisaab closed the pull request at:
https://github.com/apache/cloudstack/pull/222
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98479757
@remibergsma Hi Remi, thanks for testing. Yes, we should expect the same
states - as Abhi shared no change should be seen from the users' side. The
changes I did
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98471837
@abhinandanprateek thanks abhi, you're right on the spot. Merging the
change soon (though as you said it does not change anything functionally, but
only
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98464228
thanks for summarizing Abhi,
+1 from me
On Sun, May 3, 2015 at 4:40 AM, Abhinandan Prateek notificati...@github.com
wrote:
The
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98482158
Cool, thanks!
Sent from my iPhone
On 03 May 2015, at 15:14, Rohit Yadav notificati...@github.com wrote:
@remibergsma Hi Remi, thanks
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98372480
@koushik-das not needed. The null check is needed to prevent npe. The
unknown status will fall through.
On Sat, 2 May 2015 17:45 koushik-das
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98374391
@DaanHoogland It is not only to prevent NPE but also to go to the next item
in the list when the status cannot be determined. Think of it like this -
status
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98360696
@remibergsma thanks man! I'm too working this weekend in order to test ACS
and cut a 4.5.1 RC candidate on monday.
@koushik-das I've not touched any other
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98372263
I thought the intent of introducing 'Unkonwn' host status was to replace
the entire null semantics that was thought to be confusing in the
investigators. By
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98408236
@koushik-das the new fix ensures that testIpAddress does not return null;
and there is no need to check for null and then continue
---
If your project is set up
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98430113
The issue has got entangled, let me try to summarise what each of us has
been trying to do:
The initial fix itself takes a step into right
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98146018
@DaanHoogland thanks, I've fixed the issues now. Removed dead code where
null check is not required as the method only return Unknown/Up/Down.
---
If your project
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98142160
@dahn @snuf @remibergsma @koushik-das @abhinandanprateek can anyone of you
help review this patch? It simply replaces the null semantics with the
Status.Unknown
GitHub user bhaisaab opened a pull request:
https://github.com/apache/cloudstack/pull/222
server: Introduce Unknown Status to be used in AbstractInvestigatorImpl
The PR #211 introduced changes where the abstract investigator
testIpAddress()
would return other Status, which
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98143398
@bhaisaab Remi told me he will test. LGTM except that the null check is
redundant. We could remove it
---
If your project is set up for it, you can reply to
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98181315
@bhaisaab The 'null' semantics of all investigators needs to change to
'Unknown'. Just look at all classes that implements Investigator interface.
(XS, KVM,
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98183123
Also callers for investigate()/isAgentAlive() methods needs to be fixed to
handle Unknown instead of null.
---
If your project is set up for it, you can reply
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/222#discussion_r29503805
--- Diff: server/src/com/cloud/ha/ManagementIPSystemVMInvestigator.java ---
@@ -78,7 +78,7 @@ public Boolean isVmAlive(VirtualMachine vm, Host host)
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98247992
@bhaisaab I'm preparing my lab to test your branch. Will do that in the
weekend and let you know!
---
If your project is set up for it, you can reply to this
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98250942
@koushik-das You sure this is needed for this PR to work? it seems to me
this can be future work. Nothing will be broken more then it is now.
---
If your
24 matches
Mail list logo
