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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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):**
```
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
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):**
```
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
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
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
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
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.
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
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 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
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 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
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
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
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_r39122397
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
---
@@ -189,6 +190,7 @@
private long
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
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
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
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
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
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
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
73 matches
Mail list logo
