> On Aug. 14, 2012, 10:18 p.m., Chiradeep Vittal wrote: > > awsapi/src/com/cloud/bridge/io/S3CAStorBucketAdapter.java, line 72 > > <https://reviews.apache.org/r/6473/diff/2/?file=135872#file135872line72> > > > > This logic seems suspect: if the port is not specified or if only the > > port is specified.
It should be okay because it throws a ConfigurationException if it doesn't find any IP's. I verified error with storage.root set to "castor" or "castor 80". > On Aug. 14, 2012, 10:18 p.m., Chiradeep Vittal wrote: > > awsapi/src/com/cloud/bridge/io/S3CAStorBucketAdapter.java, line 193 > > <https://reviews.apache.org/r/6473/diff/2/?file=135872#file135872line193> > > > > You might leave temporary files around here. Thanks, corrected by putting this into the block containing the "finally" that deletes the temporary file. > On Aug. 14, 2012, 10:18 p.m., Chiradeep Vittal wrote: > > awsapi/src/com/cloud/bridge/io/S3CAStorBucketAdapter.java, line 169 > > <https://reviews.apache.org/r/6473/diff/2/?file=135872#file135872line169> > > > > you should probably set file.deleteOnExit() > > Also, "file" is a bad variable name. Even tempFile is a better name I improved the name and made sure to delete the file in the "finally" block. I don't think I want deleteOnExit() because I don't want the (potentially large) files hanging around, and it causes (a small) memory usage. > On Aug. 14, 2012, 10:18 p.m., Chiradeep Vittal wrote: > > awsapi/src/com/cloud/bridge/io/S3CAStorBucketAdapter.java, line 200 > > <https://reviews.apache.org/r/6473/diff/2/?file=135872#file135872line200> > > > > cant really progress after this error? Right, if close failed the file contents aren't reliable, so this now throws an exception. > On Aug. 14, 2012, 10:18 p.m., Chiradeep Vittal wrote: > > awsapi/src/com/cloud/bridge/io/S3CAStorBucketAdapter.java, line 248 > > <https://reviews.apache.org/r/6473/diff/2/?file=135872#file135872line248> > > > > UnsupportedOperation? Yes. > On Aug. 14, 2012, 10:18 p.m., Chiradeep Vittal wrote: > > awsapi/src/com/cloud/bridge/service/core/s3/S3Engine.java, line 1383 > > <https://reviews.apache.org/r/6473/diff/2/?file=135875#file135875line1383> > > > > Why would there be an exception here? And if there is, why isn't the > > operation failing (the code continues) No reason for exception, removed catch. > On Aug. 14, 2012, 10:18 p.m., Chiradeep Vittal wrote: > > awsapi/src/com/cloud/bridge/io/S3CAStorBucketAdapter.java, line 92 > > <https://reviews.apache.org/r/6473/diff/2/?file=135872#file135872line92> > > > > ConfigurationException? Right, that's more appropriate. > On Aug. 14, 2012, 10:18 p.m., Chiradeep Vittal wrote: > > awsapi/src/com/cloud/bridge/io/S3CAStorBucketAdapter.java, line 80 > > <https://reviews.apache.org/r/6473/diff/2/?file=135872#file135872line80> > > > > This would be a ConfigurationException? Right, that's more appropriate. > On Aug. 14, 2012, 10:18 p.m., Chiradeep Vittal wrote: > > awsapi/src/com/cloud/bridge/io/S3CAStorBucketAdapter.java, line 73 > > <https://reviews.apache.org/r/6473/diff/2/?file=135872#file135872line73> > > > > magic number? Make it a final Made constants finals. > On Aug. 14, 2012, 10:18 p.m., Chiradeep Vittal wrote: > > awsapi/src/com/cloud/bridge/io/S3CAStorBucketAdapter.java, line 55 > > <https://reviews.apache.org/r/6473/diff/2/?file=135872#file135872line55> > > > > Can you make sure your code follows these conventions: > > > > https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions Sorry it should now conform. - Jamshid ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6473/#review10312 ----------------------------------------------------------- On Aug. 15, 2012, 2:29 a.m., Jamshid Afshar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6473/ > ----------------------------------------------------------- > > (Updated Aug. 15, 2012, 2:29 a.m.) > > > Review request for cloudstack. > > > Description > ------- > > Below is the commit message. This is my first patch, let me know if I did > anything wrong or if e.g. using "storage.root" is not how configuring a new > storage backend should be done. > > Add initial support for Caringo's CAStor object storage as the S3 backend. > > Similar to the s3-hdfs example. Now storage.root can specify "castor" > followed by a list of IP addresses for the nodes in the CAStor > cluster. S3 operations will then create and read buckets and objects > in CAStor instead of a file system. > > > Diffs > ----- > > NOTICE 8a3b0bf > awsapi/src/com/cloud/bridge/io/S3CAStorBucketAdapter.java PRE-CREATION > awsapi/src/com/cloud/bridge/model/SHost.java 874b095 > awsapi/src/com/cloud/bridge/service/controller/s3/ServiceProvider.java > 7a36a4b > awsapi/src/com/cloud/bridge/service/core/s3/S3Engine.java e8b73a4 > deps/awsapi-lib/CAStorSDK-LICENSE.txt PRE-CREATION > deps/awsapi-lib/CAStorSDK.jar PRE-CREATION > deps/awsapi-lib/jmdns-2.1.jar PRE-CREATION > > Diff: https://reviews.apache.org/r/6473/diff/ > > > Testing > ------- > > Tested a boto script I believe we got from Chiradeep (localhost_test.py) that > creates buckets and streams, does a range read and delete. I will continue to > do more testing > (http://wiki.cloudstack.org/display/QA/How+to+run+S3+Tests+against+CloudStack+S3+Implementation). > > > Thanks, > > Jamshid Afshar > >
