> 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 > >
