You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrew Gaul <no...@github.com> on 2014/08/06 07:54:25 UTC
[jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
This allows callers to read the content length during container
listing. Tested against: atmosonline, aws-s3, azureblob, filesystem,
and transient. Intentionally not implemented for legacy swift
provider.
You can merge this Pull Request by running:
git pull https://github.com/andrewgaul/jclouds storage-metadata-object-size
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/465
-- Commit Summary --
* JCLOUDS-654: Add object size to StorageMetadata
-- File Changes --
M apis/atmos/src/main/java/org/jclouds/atmos/AtmosAsyncClient.java (1)
M apis/atmos/src/main/java/org/jclouds/atmos/blobstore/functions/DirectoryEntryListToResourceMetadataList.java (9)
M apis/atmos/src/main/java/org/jclouds/atmos/domain/DirectoryEntry.java (39)
M apis/atmos/src/main/java/org/jclouds/atmos/domain/SystemMetadata.java (2)
M apis/atmos/src/main/java/org/jclouds/atmos/options/ListOptions.java (4)
M apis/atmos/src/main/java/org/jclouds/atmos/xml/ListDirectoryResponseHandler.java (16)
M apis/atmos/src/test/java/org/jclouds/atmos/AtmosAsyncClientTest.java (4)
M apis/atmos/src/test/java/org/jclouds/atmos/blobstore/functions/ResourceMetadataListToDirectoryEntryList.java (2)
M apis/atmos/src/test/java/org/jclouds/atmos/functions/ParseDirectoryListFromContentAndHeadersTest.java (4)
M apis/atmos/src/test/java/org/jclouds/atmos/xml/ListDirectoryResponseHandlerTest.java (4)
M apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java (1)
M apis/s3/src/main/java/org/jclouds/s3/blobstore/functions/ObjectToBlobMetadata.java (1)
M apis/swift/src/test/java/org/jclouds/openstack/swift/blobstore/integration/SwiftContainerIntegrationLiveTest.java (6)
M blobstore/src/main/java/org/jclouds/blobstore/LocalAsyncBlobStore.java (1)
M blobstore/src/main/java/org/jclouds/blobstore/TransientStorageStrategy.java (1)
M blobstore/src/main/java/org/jclouds/blobstore/attr/ContainerCapability.java (1)
M blobstore/src/main/java/org/jclouds/blobstore/domain/MutableStorageMetadata.java (3)
M blobstore/src/main/java/org/jclouds/blobstore/domain/StorageMetadata.java (3)
M blobstore/src/main/java/org/jclouds/blobstore/domain/internal/MutableStorageMetadataImpl.java (11)
M blobstore/src/main/java/org/jclouds/blobstore/domain/internal/StorageMetadataImpl.java (19)
M blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseContainerIntegrationTest.java (26)
M providers/azureblob/src/main/java/org/jclouds/azureblob/blobstore/functions/BlobPropertiesToBlobMetadata.java (1)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/465.patch
https://github.com/jclouds/jclouds/pull/465.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
Sorry for my delayed response; I have addressed all actionable review comments.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465#issuecomment-68799556
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Phillips <no...@github.com>.
> @@ -49,11 +53,17 @@ public void endElement(String uri, String name, String qName) {
> } else if (qName.equals("FileType")) {
> currentType = FileType.fromValue(currentText.toString().trim());
> } else if (qName.equals("Filename")) {
> + currentFileName = currentText.toString().trim();
> + if (currentFileName.equals(""))
> + currentFileName = null;
> + } else if (qName.equals("Name")) {
> currentName = currentText.toString().trim();
Where is `currentName` used now, actually?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r16132115
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
> @@ -94,4 +95,9 @@ public void testCreateDeleteContainerMetadata() throws InterruptedException {
> returnContainer(containerName);
> }
> }
> +
> + @Test(groups = { "integration", "live" })
> + public void testListContainerGetBlobSize() throws Exception {
> + throw new SkipException("Intentionally not implemented for the legacy Swift provider");
Added comment, "use new Swift provider instead".
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r22496906
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
> @@ -57,7 +59,7 @@ public ListOptions limit(int maxresults) {
> * limit.
> */
> public ListOptions includeMeta() {
> - headers.put("x-emc-include-meta", Integer.toString(1));
> + headers.replaceValues("x-emc-include-meta", ImmutableList.of(Integer.toString(1)));
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r22495967
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -332,6 +334,30 @@ public void testListContainer() throws InterruptedException, ExecutionException,
>
> }
>
> + @Test(groups = { "integration", "live" })
> + public void testListContainerGetBlobSize() throws Exception {
> + String containerName = getContainerName();
> + try {
> + ByteSource byteSource = ByteSource.wrap(new byte[42]);
42! :+1:
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r16130447
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Phillips <no...@github.com>.
> super(id, name, location, uri, userMetadata);
> this.eTag = eTag;
> this.creationDate = creationDate;
> this.lastModified = lastModified;
> this.type = checkNotNull(type, "type");
> + this.size = size;
> + }
> +
> + /** @deprecated use 10-argument variant of constructor. */
Can we use a Javadoc link rather than "10-argument variant of constructor"
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r16132252
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
> currentName = currentText.toString().trim();
> - if (currentName.equals(""))
> - currentName = null;
> + } else if (qName.equals("Value")) {
> + if (currentName.equals("size")) {
I cannot find sufficient mention of case-sensitivity in the Atmos documentation to motivate this change. If you can find additional references we can change the entire provider to this scheme.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r22497391
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
> @@ -62,6 +62,7 @@
> */
> MILLISECOND_PRECISION,
>
> + // TODO: how to use this?
Removed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r22495860
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
> super(id, name, location, uri, userMetadata);
> this.eTag = eTag;
> this.creationDate = creationDate;
> this.lastModified = lastModified;
> this.type = checkNotNull(type, "type");
> + this.size = size;
> + }
> +
> + /** @deprecated use 10-argument variant of constructor. */
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r22497194
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Shri Javadekar <no...@github.com>.
> currentName = currentText.toString().trim();
> - if (currentName.equals(""))
> - currentName = null;
> + } else if (qName.equals("Value")) {
> + if (currentName.equals("size")) {
The way you have it, "size" is in the correct case as per [this](https://community.emc.com/servlet/JiveServlet/previewBody/3481-102-6-16527/Atmos%20Online%20Programmer's%20Guide%201.2.4A.pdf;jsessionid=360D5B1A4D136E51874AB885545A7FA4.node0). However, wouldn't it be better if this was a case insensitive comparison?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r16152290
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Shri Javadekar <no...@github.com>.
Generally, looks good. I'll take a look again when other concerns are addressed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465#issuecomment-51996666
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds #1459](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1459/) 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/465#issuecomment-51299586
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
> @@ -72,9 +72,9 @@ public void testWithoutToken() {
> protected Set<DirectoryEntry> values() {
> Builder<DirectoryEntry> expected = ImmutableSet.builder();
> expected.add(new DirectoryEntry("4980cdb2a411106a04a4538c92a1b204ad92077de6e3", FileType.DIRECTORY,
> - "adriancole-blobstore-2096685753"));
> + "adriancole-blobstore-2096685753", 0));
Agreed although this is outside the scope of this pull request.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r22495826
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Shri Javadekar <no...@github.com>.
Sorry I missed this earlier. I will take a look at this later today.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465#issuecomment-51953773
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
> @@ -49,11 +53,17 @@ public void endElement(String uri, String name, String qName) {
> } else if (qName.equals("FileType")) {
> currentType = FileType.fromValue(currentText.toString().trim());
> } else if (qName.equals("Filename")) {
> + currentFileName = currentText.toString().trim();
> + if (currentFileName.equals(""))
> + currentFileName = null;
> + } else if (qName.equals("Name")) {
> currentName = currentText.toString().trim();
Previously `currentName` represented the file name. Now `currentFileName` does this and we reuse `currentName` for individual metadata entries. An abbreviated listing from the Atmos programmers guide:
```
<ListObjectsResponse xmlns='http://www.emc.com/cos/'>
<Object>
<ObjectID>499ad542a2a8bc200499ad5a7099940499b44f51e97d
</ObjectID>
<SystemMetadataList>
...
<Metadata>
<Name>size</Name>
<Value>7589</Value>
</Metadata>
...
</SystemMetadataList>
...
</SystemMetadataList>
```
Not listed, the Filename and FileType since the examples only have the object interface, not the namespace interface.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r22496834
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Phillips <no...@github.com>.
> @@ -57,7 +59,7 @@ public ListOptions limit(int maxresults) {
> * limit.
> */
> public ListOptions includeMeta() {
> - headers.put("x-emc-include-meta", Integer.toString(1));
> + headers.replaceValues("x-emc-include-meta", ImmutableList.of(Integer.toString(1)));
[minor] Could/should "ImmutableList.of(Integer.toString(1))" be a constant?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r16131970
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -72,9 +72,9 @@ public void testWithoutToken() {
> protected Set<DirectoryEntry> values() {
> Builder<DirectoryEntry> expected = ImmutableSet.builder();
> expected.add(new DirectoryEntry("4980cdb2a411106a04a4538c92a1b204ad92077de6e3", FileType.DIRECTORY,
> - "adriancole-blobstore-2096685753"));
> + "adriancole-blobstore-2096685753", 0));
FWIW, I feel that developer names should be stripped from the codebase in favor of either "jclouds", "jclouds-user", or just "user". This applies to all instances in this PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r16130686
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
Pushed to master as fae097e144c0b9adf00f88ded1d614e2aec963f8.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465#issuecomment-69445834
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Jeremy Daggett <no...@github.com>.
> super(id, name, location, uri, userMetadata);
> this.eTag = eTag;
> this.creationDate = creationDate;
> this.lastModified = lastModified;
> this.type = checkNotNull(type, "type");
> + this.size = size;
> + }
> +
> + /** @deprecated use 10-argument variant of constructor. */
> + @Deprecated
> + public StorageMetadataImpl(StorageType type, @Nullable String id, @Nullable String name,
> + @Nullable Location location, @Nullable URI uri, @Nullable String eTag,
> + @Nullable Date creationDate, @Nullable Date lastModified,
> + Map<String, String> userMetadata) {
> + this(type, id, name, location, uri, eTag, creationDate, lastModified, userMetadata, /*size=*/ null);
`/*size=*/` in the method signature should be removed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r16130434
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Jeremy Daggett <no...@github.com>.
@andrewgaul +1 Just a few comments to address and this should be good to merge.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465#issuecomment-51952940
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
> super(id, name, location, uri, userMetadata);
> this.eTag = eTag;
> this.creationDate = creationDate;
> this.lastModified = lastModified;
> this.type = checkNotNull(type, "type");
> + this.size = size;
> + }
> +
> + /** @deprecated use 10-argument variant of constructor. */
> + @Deprecated
> + public StorageMetadataImpl(StorageType type, @Nullable String id, @Nullable String name,
> + @Nullable Location location, @Nullable URI uri, @Nullable String eTag,
> + @Nullable Date creationDate, @Nullable Date lastModified,
> + Map<String, String> userMetadata) {
> + this(type, id, name, location, uri, eTag, creationDate, lastModified, userMetadata, /*size=*/ null);
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r22495766
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
> @@ -92,4 +92,7 @@
> */
> Date getLastModified();
>
> + // TODO: how does this interact with ContentMetadata.getContentLength?
This is the core issue with this pull request: where should size exist? All providers return it as part of the object metadata, but traditionally jclouds has stored it in the content metadata. Listing operations return the former and per-object operations return the latter. This really should only exist in the former; should we deprecate the latter?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r22497002
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Gaul <no...@github.com>.
@jdaggett @shrinandj I appreciate feedback on this approach. Presently it duplicates the object size in `StorageMetadata` and `ContentMetadata`. Most providers supply this information already, but Atmos required using a detailed listing and I chose not to implement this in the legacy Swift provider.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465#issuecomment-51296575
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Phillips <no...@github.com>.
> @@ -94,4 +95,9 @@ public void testCreateDeleteContainerMetadata() throws InterruptedException {
> returnContainer(containerName);
> }
> }
> +
> + @Test(groups = { "integration", "live" })
> + public void testListContainerGetBlobSize() throws Exception {
> + throw new SkipException("Intentionally not implemented for the legacy Swift provider");
Perhaps explain _why_ not implemented? Does it not work? Does it take too long? Other..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r16132172
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Andrew Phillips <no...@github.com>.
> @@ -92,4 +92,7 @@
> */
> Date getLastModified();
>
> + // TODO: how does this interact with ContentMetadata.getContentLength?
Do we need this TODO?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r16132202
Re: [jclouds] JCLOUDS-654: Add object size to StorageMetadata (#465)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -62,6 +62,7 @@
> */
> MILLISECOND_PRECISION,
>
> + // TODO: how to use this?
Extra comment?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/465/files#r16130759