cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Prachi Damle" <prachi.da...@citrix.com>
Subject Re: Review Request 21776: AWSAPI: cloudstack api on POST requests (instead GET), ssl enabling fixed
Date Wed, 16 Jul 2014 23:42:43 GMT


> On June 24, 2014, 7:28 p.m., Prachi Damle wrote:
> > Hi,
> > 
> > 1) EC2Engine.java: I see a lot of new line breaks introduced and its hard to know
what the changes are.
> > Is it possible to clean this or provide a list of changes/line numbers changed?
> > 
> > 2) CloudStackClient.java: 
> > What are the reasons for:
> > - Deletion of JsonAccessor.java and using JsonElement instead
> > - Changes in CloudStackClient::call() method around error handling
> > 
> > 3)CloudStackApi.java:
> > - What are the reasons to replace CloudStackCommand by CloudStackQueryBuilder?
> >
> 
> Dmitry Batkovich wrote:
>     Hello
>     
>     1) I don't see any newlines symbols in EC2Engine.java https://reviews.apache.org/r/21776/diff/#index_header
>     
>     2) -reasons for deletion JsonAccessor - this class has provided "comfortable" access
to json element values, but was non-static class (If you look at code it looks strange that
this class is not-static) and used some matching pattern in raw string form (why not some
DSL if you really need?). Hence someone who will really needs this functionality has small
but unpleasant reduction of performance. And I rewrite this as static class without "matching
patterns".
>      
>       -avoiding "throws Exception" or "catch Exception" in future
>     
>     
>     3) Ok, I can revert change name if you want

Ok I see the patterns used in JsonAccessor.

Was there any testing with EC2 APIs using any tools done?
Since this patch is changing the mechanism of making the request to CloudStack and handling
the response, I think this needs to be tested against CloudStack for variety of EC2 APIs.


- Prachi


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


On May 27, 2014, 8:04 p.m., Dmitry Batkovich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21776/
> -----------------------------------------------------------
> 
> (Updated May 27, 2014, 8:04 p.m.)
> 
> 
> Review request for cloudstack, Chiradeep Vittal, daan Hoogland, and Prachi Damle.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> * CloudStackClient.java http request mechanism replaced from GET requests to POST for
supporting EC2 requests larger than 2KB
> * SSL enabling fixed in EC2Engine.java
> 
> continuation of https://reviews.apache.org/r/17586/
> 
> 
> Diffs
> -----
> 
>   awsapi/conf/ec2-service.properties.in 82f5ad8 
>   awsapi/src/com/cloud/bridge/service/core/ec2/EC2Engine.java cd20214 
>   awsapi/src/com/cloud/bridge/util/JsonAccessor.java 2a94dea 
>   awsapi/src/com/cloud/bridge/util/JsonElementUtil.java PRE-CREATION 
>   awsapi/src/com/cloud/stack/CloudStackApi.java b7a1210 
>   awsapi/src/com/cloud/stack/CloudStackClient.java 03eba96 
>   awsapi/src/com/cloud/stack/CloudStackClientException.java PRE-CREATION 
>   awsapi/src/com/cloud/stack/CloudStackCommand.java 8d6aa68 
>   awsapi/src/com/cloud/stack/CloudStackQueryBuilder.java PRE-CREATION 
>   awsapi/test/com/cloud/gate/util/CloudStackClientTestCase.java 826cb3a 
>   awsapi/test/com/cloud/gate/util/JsonAccessorTestCase.java 8603e59 
>   awsapi/test/com/cloud/gate/util/JsonElementUtilTestCase.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21776/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dmitry Batkovich
> 
>


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