cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ilia Shakitko" <i.shaki...@leaseweb.com>
Subject Re: Review Request 23805: Added "state" parameter to the "listPublicIpAddresses" API call
Date Mon, 28 Jul 2014 13:25:18 GMT


> On July 28, 2014, 12:43 p.m., Rohit Yadav wrote:
> > api/src/org/apache/cloudstack/api/command/user/address/ListPublicIpAddressesCmd.java,
lines 205-207
> > <https://reviews.apache.org/r/23805/diff/1/?file=639247#file639247line205>
> >
> >     This was removed.

I deleted it because it's not being used in the code. There is a second method: "public Boolean
isAllocatedOnly()" this one is used instead.


> On July 28, 2014, 12:43 p.m., Rohit Yadav wrote:
> > api/src/org/apache/cloudstack/api/command/user/address/ListPublicIpAddressesCmd.java,
lines 156-166
> > <https://reviews.apache.org/r/23805/diff/1/?file=639247#file639247line156>
> >
> >     I guess moving it above was a cosmetic change. Please fix that you don't remove/change
anything in the move refactor.

Is this still the case according to the written comments below?


> On July 28, 2014, 12:43 p.m., Rohit Yadav wrote:
> > api/src/org/apache/cloudstack/api/command/user/address/ListPublicIpAddressesCmd.java,
lines 156-167
> > <https://reviews.apache.org/r/23805/diff/1/?file=639247#file639247line156>
> >
> >     I guess the move refactor was a cosmetic change, but it removed getAllocatedOnly()
method. Please fix it, or explain why we need to remove it?

Because it's a duplicated method, isn't it? As well is "isForLoadBalancing" and "getForLoadBalancing"
(just noticed, I'll check for these methods usages as well later)


- Ilia


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23805/#review48842
-----------------------------------------------------------


On July 28, 2014, 10:12 a.m., Ilia Shakitko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23805/
> -----------------------------------------------------------
> 
> (Updated July 28, 2014, 10:12 a.m.)
> 
> 
> Review request for cloudstack, Alena Prokharchyk, Alex Huang, Harikrishna Patnala, Kishan
Kavala, Prachi Damle, Rohit Yadav, Ilia Shakitko, Wei Zhou, and Wido den Hollander.
> 
> 
> Bugs: CLOUDSTACK-7159
>     https://issues.apache.org/jira/browse/CLOUDSTACK-7159
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> This improvement is introducing a new parameter for the "listPublicIpAddresses" API call
- "state".
> 
> Few times we've faced an impedemence of having a list of "Free" publicIpAddresses. You
have to go thru all the IPs with (allocatedonly = false) and filter out the "Free" once. It's
not a big deal, but it's an extra time and traffic between CloudStack and an API consumer.
> 
> I also moved few methods out of the 'API Implementation' and put them above as a minor
refactoring.
> Method "getForLoadBalancing" has been removed because it's not being used in code anywhere
else.
> 
> This patch is done for "master" branch.
> 
> 
> Diffs
> -----
> 
>   api/src/org/apache/cloudstack/api/command/user/address/ListPublicIpAddressesCmd.java
07ccfe9 
>   server/src/com/cloud/server/ManagementServerImpl.java 99b12732 
> 
> Diff: https://reviews.apache.org/r/23805/diff/
> 
> 
> Testing
> -------
> 
> 1) Build successfull
> 2) No tests broken
> 3) Tested few different calls with cloudmonkey:
> 
> list publicipaddresses forvirtualnetwork=false listall=true page=1 pagesize=0
> count = 10
> 
> list publicipaddresses forvirtualnetwork=false listall=true allocatedonly=false page=1
pagesize=0
> count = 100
> 
> list publicipaddresses forvirtualnetwork=false listall=true state=free page=1 pagesize=0
> count = 90
> 
> 
> Thanks,
> 
> Ilia Shakitko
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message