You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by jkew <no...@github.com> on 2013/07/12 01:44:35 UTC
[jclouds] Minor cleanup of Azureblob (#66)
Cleanup of Azureblob code; as per pull request review. Additional items not covered here are assigned specific bug ids.
You can merge this Pull Request by running:
git pull https://github.com/jkew/jclouds master
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/66
-- Commit Summary --
* Cleanup of Azureblob code; as per pull request review. Additional items
-- File Changes --
M providers/azureblob/src/main/java/org/jclouds/azureblob/blobstore/strategy/AzureBlobBlockUploadStrategy.java (5)
M providers/azureblob/src/main/java/org/jclouds/azureblob/domain/internal/BlobBlockPropertiesImpl.java (3)
M providers/azureblob/src/main/java/org/jclouds/azureblob/predicates/validators/BlockIdValidator.java (4)
M providers/azureblob/src/main/java/org/jclouds/azureblob/xml/BlobBlocksResultsHandler.java (2)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/66.patch
https://github.com/jclouds/jclouds/pull/66.diff
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by jkew <no...@github.com>.
I was planning on doing the Nullable change; but I've been backlogged at work. I should be able to get to it tonight.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66#issuecomment-21091592
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by Andrew Phillips <no...@github.com>.
> @@ -33,8 +33,8 @@
> @Singleton
> public class BlockIdValidator extends Validator<String> {
> @Override
> - public void validate(@Nullable String s) throws IllegalArgumentException {
> - if (s.length() > 64)
> + public void validate(String s) throws IllegalArgumentException {
Hm, if we remove the annotation wouldn't the assumption be (as elsewhere in jclouds) that `s` is **not** `null`, which would make the `null` check seem superfluous? I think we should either have the annotation and the null check or drop the annotation, assume that `s` cannot be null and drop the null check.
If the latter assumption is incorrect, I'd vote for the annotation and the null check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66/files#r5156816
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by Andrew Phillips <no...@github.com>.
See https://github.com/jclouds/jclouds/pull/67
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66#issuecomment-21116699
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by Andrew Phillips <no...@github.com>.
> @@ -33,8 +33,8 @@
> @Singleton
> public class BlockIdValidator extends Validator<String> {
> @Override
> - public void validate(@Nullable String s) throws IllegalArgumentException {
> - if (s.length() > 64)
> + public void validate(String s) throws IllegalArgumentException {
Based on @andrewgaul's comment, shall we then keep `@Nullable` and move on? Agreed that there are more interesting questions to solve.
Thanks, @jkew!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66/files#r5182981
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds #199](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/199/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66#issuecomment-20851809
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by Andrew Gaul <no...@github.com>.
+1, committing as-is.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66#issuecomment-21085771
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by jkew <no...@github.com>.
> @@ -33,8 +33,8 @@
> @Singleton
> public class BlockIdValidator extends Validator<String> {
> @Override
> - public void validate(@Nullable String s) throws IllegalArgumentException {
> - if (s.length() > 64)
> + public void validate(String s) throws IllegalArgumentException {
Remember that the lack of @Nullable does not prevent anyone from making a call to this function with a null value. Whether the code itself does anything with null is besides the point (except when a tool infers the nullability of a parameter).
@Nullable used for static analysis and sometimes used by an ioc container for injection (which is not the case here).
I'd keep the @Nullable out; because it at least lets the ide alert developers to when they may be doing something terribly wrong (like passing an invalid argument to a function whose only purpose is to throw IllegalStateExceptions if a null occurs).
In any case; I'll just do whatever you want - because honestly this is not a very useful or interesting problem.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66/files#r5178105
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by Andrew Gaul <no...@github.com>.
Committed to master. I also tested multipart locally and it works for my use case. @jkew do you have any follow-on commits or should I backport to 1.6.x?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66#issuecomment-21085834
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by Andrew Gaul <no...@github.com>.
> @@ -57,7 +58,7 @@ public AzureBlobBlockUploadStrategy(AzureBlobClient client, PayloadSlicer slicer
> public String execute(String container, Blob blob) {
> String blobName = blob.getMetadata().getName();
> Payload payload = blob.getPayload();
> - long length = payload.getContentMetadata().getContentLength();
> + Long length = payload.getContentMetadata().getContentLength();
We can retain the check and use a primitive via the return value from checkNotNull:
```
long length = checkNotNull(payload.getContentMetadata().getContentLength(),
"please invoke payload.getContentMetadata().setContentLength(length) prior to azure block upload");
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66/files#r5156030
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by Andrew Phillips <no...@github.com>.
> @@ -81,7 +82,7 @@ public String execute(String container, Blob blob) {
> blockIds.add(blockId);
> client.putBlock(container, blobName, blockId, block);
> }
> - assert bytesWritten == length;
> + checkState(bytesWritten == length, "Wrote " + bytesWritten + " bytes, but we wanted to write " + length + " bytes");
For what it's worth, this can also be written using the ["placeholder form" of `checkState`](http://guava-libraries.googlecode.com/svn-history/r14/trunk/javadoc/com/google/common/base/Preconditions.html#checkState\(boolean, java.lang.String, java.lang.Object...\)) as
```
checkState(bytesWritten == length, "Wrote %s bytes, but we wanted to write %s bytes", bytesWritten, length);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66/files#r5156785
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by jkew <no...@github.com>.
> @@ -33,8 +33,8 @@
> @Singleton
> public class BlockIdValidator extends Validator<String> {
> @Override
> - public void validate(@Nullable String s) throws IllegalArgumentException {
> - if (s.length() > 64)
> + public void validate(String s) throws IllegalArgumentException {
I removed it because; technically it can't; it would immediately throw an IllegalArgumentException. Null isn't valid. The only reason it was in there was because the interface has that on there; but I don't think that's even the correct annotation for the interface in most circumstances.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66/files#r5156293
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by Andrew Phillips <no...@github.com>.
> @@ -57,7 +58,7 @@ public AzureBlobBlockUploadStrategy(AzureBlobClient client, PayloadSlicer slicer
> public String execute(String container, Blob blob) {
> String blobName = blob.getMetadata().getName();
> Payload payload = blob.getPayload();
> - long length = payload.getContentMetadata().getContentLength();
> + Long length = payload.getContentMetadata().getContentLength();
Apologies if I missed an earlier discussion about the desirability of primitives here, but if `getContentLength` actually [returns a `Long`](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/io/ContentMetadata.java#L55) would it not be more straightforward to assign it to a `Long`?
Am fine with either alternative, by the way...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66/files#r5156763
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #63](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/63/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66#issuecomment-20851986
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by Andrew Gaul <no...@github.com>.
> @@ -33,8 +33,8 @@
> @Singleton
> public class BlockIdValidator extends Validator<String> {
> @Override
> - public void validate(@Nullable String s) throws IllegalArgumentException {
> - if (s.length() > 64)
> + public void validate(String s) throws IllegalArgumentException {
Shouldn't this continue to be @Nullable?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66/files#r5156039
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #521](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/521/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66#issuecomment-20851956
Re: [jclouds] Minor cleanup of Azureblob (#66)
Posted by Andrew Gaul <no...@github.com>.
Closed #66.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/66