Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1444
---
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 is
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-218579831
I think this one is pretty much ready. I have one code review on this one.
Can I get one more person to look over this for me? Thanks...
---
If your project is se
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-218579019
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 1
Errors: 0
Duration: 9h 12m 09s
```
**Summary of the p
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-217798010
Thanks @rafaelweingartner for the updated PR.
LGTM based on code review and verification of PR on simulator.
---
If your project is set up for it, you can r
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-217537499
@swill done ;)
---
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 ha
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-217479698
Can we get another code review on this one. Also, can you force push to
kick off Jenkins again? Thx...
---
If your project is set up for it, you can reply to this
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216957591
@koushik-das, You are right.
Those variables that were introduced are all primitives; if they have not
being loaded, their default value will be zero.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r62006414
--- Diff: server/src/com/cloud/api/query/dao/UserVmJoinDaoImpl.java ---
@@ -205,14 +205,21 @@ public UserVmResponse newUserVmResponse(ResponseView
vi
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216597727
That is a nice suggestion.
I have done that, what do you think now?
---
If your project is set up for it, you can reply to this email and have your
r
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216569013
@rafaelweingartner just checked that the first commit indeed is by someone
else, though I think it would be still valid to note in your commit (the 2nd
one) the JIRA
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r61870927
--- Diff: server/src/com/cloud/api/query/dao/UserVmJoinDaoImpl.java ---
@@ -205,14 +205,21 @@ public UserVmResponse newUserVmResponse(ResponseVi
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216508737
@rhtyd, I understand and I agree with your concerns. The point here is that
I am not fragmenting commits, I just cherry picked a commit that was reverted
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r61844330
--- Diff: server/src/com/cloud/api/query/dao/UserVmJoinDaoImpl.java ---
@@ -205,14 +205,21 @@ public UserVmResponse newUserVmResponse(ResponseView
vi
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216436317
@rafaelweingartner for a project with thousands of commits, splitting the
commits for a PR or bug that solves for the same logical issue results in
fragmented history
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216266457
I think the two commits are fine in this case, so just leave it as it is.
ð
Can we get some LGTM code reviews on this one? Thanks...
---
If your proje
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216264551
@rhtyd I think it would not be a good idea to squash these commits. We will
lose the history if we do it here.
I only cherry picked the changes do
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216226029
@rafaelweingartner please rebase against master, and squash changes to a
single commit
tag:easypr
---
If your project is set up for it, you can reply to thi
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215847599
ya, that is interesting. a couple people have mentioned similar tools.
once I get the repo moved to the new github org I can start look into adding
such tooling...
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215833023
Ok, I will do that.
I can help youwith that (If we get access to the VM).
It would be nice, something like this plugin
"https://github.com/janinko/
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215832199
Just do a force push again. Jenkins is being a bit of a princess recently,
so we just have to keep trying. This has been happening to a lot of PRs and it
is almost
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215829693
@swill Jenkins is complaining about a file called "testsmallfileinactive",
but I have not introduced any sort of file like that. Is that some sort of
tras
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215822574
@swill conflicts solved
---
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 do
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215554170
@rafaelweingartner please rebase as we currently have merge conflicts.
Thanks...
---
If your project is set up for it, you can reply to this email and have your
rep
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215122425
I would like to get at least one LGTM in this PR. The code is basically
the same as the previous one other than the fixes to the potential NPE issues.
I think we sh
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215111745
@swill, I am so sorry, I had forgotten this PR. I opened the email to help
in a review thinking that this was from someone else.
Are you sure we c
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215109078
Ran the usage tests to make sure everything is passing. Don't worry about
the snapshot issue, it is not related to this PR.
I think this PR is ready if I cou
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215108116
### CI RESULTS
```
Tests Run: 18
Skipped: 0
Failed: 0
Errors: 2
```
**Summary of the problem(s):**
```
E
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215094461
The issues that we see here are environment issues that sometimes show up.
They are not related to this PR.
I am running another set of usage specific inte
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215090037
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 3
Errors: 0
```
**Summary of the problem(s):**
```
F
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r58480834
--- Diff:
plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
---
@@ -484,6 +489,10 @@ public DomainBlockStat
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r58375427
--- Diff:
plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
---
@@ -484,6 +489,10 @@ public Dom
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r58316653
--- Diff:
plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
---
@@ -484,6 +489,10 @@ public DomainBlockStat
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-200473159
Can we continue the conversation at #1444 please. This commit has been
reverted and has been pulled into #1444 to continue QA of the code. Please
come contribute at
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-199497512
Hey Guys, Sorry I have been MIA on this. I have been traveling and have
limited connectivity.
I am going to revert this merge. There are visible problems w
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-199270124
Guys, LGTMs or not, there were no integration test results posted so it
shouldn't have been merged.
---
If your project is set up for it, you can reply to this
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-199254175
@DaanHoogland I don't see any reason for closing master in this case. If an
issue is noticed after a merge, it definitely needs to be tracked and fixed.
Releasin
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-199227949
@kishankavala you are formally correct and we had not closed master for
release so I don't blame you for the merge. The issue as brought up by @swill
is not to
Github user kishankavala commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-199222784
@swill @remibergsma @DaanHoogland PR was open since Sep 2015. Review from
@swill came after the PR was merged on Mar 17 2016. By then, there were code
reviews a
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-199209886
@swill I would not allow any other merge before this is fixed or reverted!
---
If your project is set up for it, you can reply to this email and have your
reply
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-198515978
@swill Please do proceed with revert, I totally agree.
`git revert -m 1 dc0ba6bd1a774d3ff4bc4a4dcc00e1434ab1f6e3` will do. Let me
know if you need help.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-197904412
@swill you can create a PR if you want.
---
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 p
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/780
---
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 is
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r56508756
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
---
@@ -3026,11 +3031,16 @@ public VmStat
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-197890163
I see this was merged into master this morning. Hmmm...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r56509710
--- Diff:
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java
---
@@ -3332,7 +3332,14 @@ public P
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-197939345
@rafaelweingartner I don't mind waiting a bit, but runtime exceptions need
to be taken seriously. As the 4.9 RM, it is my responsibility to make sure
this type of stu
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r56508389
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
---
@@ -3026,11 +3031,16 @@ public VmStat
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-197932674
@swill, I understand your position; as you, I also think that there was
room for improvements in this PR (especially to avoid possible runtime
exception).
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-197917819
@ustcweizhou the point is more that there are things in this PR that I am
not comfortable with and as the 4.9 RM, I need to be comfortable with the code
that goes into
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-197893296
should we revert this merge in favor of fixing these issues?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r56506811
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
---
@@ -3115,6 +3125,9 @@ public VmStatsEntry getVmSt
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r56509815
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -4879,10 +4889,16 @@ private VirtualMa
Github user kishankavala commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-197262533
Tested Manually. memorykbs, memoryintfreekbs,memorytargetkbs are listed
correctly.
LGTM.
`1a574e9d1-8057-4195-a8b3-2117e9059652VM-a574e9d1-8057
Github user maneesha-p commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r56314421
--- Diff:
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java
---
@@ -3332,7 +3332,14 @@ public PowerSta
Github user kishankavala commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r56311601
--- Diff:
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java
---
@@ -3332,7 +3332,14 @@ public PowerS
Github user maneesha-p commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-188171070
@remibergsma @wido @bhaisaab 2 LGTMs can it be merged?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-176693950
This looks good to me. Hard to test though :(
---
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 pr
Github user maneesha-p commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-176683204
@bhaisaab @wido @remibergsma Plz review.
---
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 p
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-175665865
@maneesha-p please rebase against latest branch and resubmit
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user maneesha-p commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-139961981
@wido @remibergsma I am sorry I changed it for kvm but updated the
description wrong.
---
If your project is set up for it, you can reply to this email and have
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-139915515
@remibergsma @maneesha-p Indeed. We have the information, so why not KVM?
That would cover allmost all cases.
---
If your project is set up for it, you can reply to th
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-139296681
@maneesha-p Why not for KVM?
---
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
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-139109578
I see tests only for LibrvirtResource. They are missing for StatsCollector,
CitrixResourceBase, VmwareResource
---
If your project is set up for it, you can reply
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r39122432
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -4662,8 +4663,14 @@ private VirtualMachineGuest
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r39122397
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
---
@@ -189,6 +190,7 @@
private long _hvV
Github user maneesha-p commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r39034863
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -4751,7 +4760,7 @@ private VirtualMachineGues
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r38917337
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -4751,7 +4760,7 @@ private VirtualMachineGuestO
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r38917202
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -4663,7 +4663,7 @@ private VirtualMachineGuestO
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/780#discussion_r38916972
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
---
@@ -2981,11 +2985,16 @@ public VmStatsEntry ge
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/780#issuecomment-138524943
LGTM (though not tested in an environment), anyone wants to review @wido
@remibergsma @wilderrodrigues ?
---
If your project is set up for it, you can reply to thi
GitHub user manuiiit opened a pull request:
https://github.com/apache/cloudstack/pull/780
CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include
memory utilization information for a VM
for xenserver,hyperv and for vmware.
You can merge this pull request into a Git r
Github user manuiiit closed the pull request at:
https://github.com/apache/cloudstack/pull/774
---
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 i
GitHub user manuiiit opened a pull request:
https://github.com/apache/cloudstack/pull/774
CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include
memory utilization information for a VM
for xenserver,hyperv and for vmware
You can merge this pull request into a Git re
73 matches
Mail list logo
