cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jamshid Afshar" <jamshid.afs...@caringo.com>
Subject Re: Review Request: Add initial support for Caringo's CAStor object storage as the S3 backend.
Date Wed, 15 Aug 2012 02:39:52 GMT


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


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