[jira] [Created] (JCLOUDS-1345) testMetadata test is failing if we use ApacheHCHttpCommandExecutorServiceModule

[Chaithanya Ganta (JIRA)]

Chaithanya Ganta created JCLOUDS-1345:
-

 Summary: testMetadata test is failing if we use 
ApacheHCHttpCommandExecutorServiceModule
 Key: JCLOUDS-1345
 URL: https://issues.apache.org/jira/browse/JCLOUDS-1345
 Project: jclouds
  Issue Type: Bug
  Components: jclouds-blobstore
Reporter: Chaithanya Ganta


testMetadata (in BaseBlobIntegrationTest) test is failing if we use 
ApacheHCHttpCommandExecutorServiceModule, passing with 
JavaUrlHttpCommandExecutorServiceModule.

Command used to run test:
mvn integration-test -pl :azureblob -Plive -Dtest.azureblob.identity="" 
-Dtest.azureblob.credential="" -Dtest=AzureBlobIntegrationLiveTest#testMetadata


java.lang.AssertionError: application/unknown
at 
org.jclouds.blobstore.integration.internal.BaseBlobIntegrationTest.validateMetadata(BaseBlobIntegrationTest.java:1388)
at 
org.jclouds.blobstore.integration.internal.BaseBlobIntegrationTest.testMetadata(BaseBlobIntegrationTest.java:872)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:85)
at org.testng.internal.Invoker.invokeMethod(Invoker.java:696)
at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:882)
at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1189)
at 
org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:124)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

@andrewgaul  Currently, I don't have any performance numbers with me. As I have 
mentioned before, we were comparing the no. of requests being made for a 
multi-part upload using S3Proxy vs using Azure directly, where I've encountered 
this redundant request and raised the PR to fix the same.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-320612582

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

@andrewgaul This PR doesn't introduce any regression. `putBlob` with 
zero-length inputstream is failing before as well on google-cloud. The thing is 
we don't have a test case for that scenario before, so you didn't see any test 
failure.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-320528041

[jira] [Commented] (JCLOUDS-1327) putBlob with zero length Inputstream is failing on google-cloud-storage

[Chaithanya Ganta (JIRA)]

[ 
https://issues.apache.org/jira/browse/JCLOUDS-1327?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16115909#comment-16115909
 ] 

Chaithanya Ganta commented on JCLOUDS-1327:
---

[~argaul] This issue has nothing to do with the proposed change in 
https://github.com/jclouds/jclouds/pull/1120. There is actually a bug in 
BaseBlobStore.putMultipartBlob which causes this issue.

> putBlob with zero length Inputstream is failing on google-cloud-storage
> ---
>
> Key: JCLOUDS-1327
> URL: https://issues.apache.org/jira/browse/JCLOUDS-1327
> Project: jclouds
>  Issue Type: Bug
>  Components: jclouds-blobstore
>    Reporter: Chaithanya Ganta
>  Labels: google-cloud-storage
>
> putBlob with zero length Inputstream is failing on google-cloud-storage with
> org.jclouds.http.HttpResponseException: command: POST 
> https://www.googleapis.com/storage/v1/b/gaul-blobstore3/o/multipart-upload/compose
>  HTTP/1.1 failed with response: HTTP/1.1 400 Bad Request; content: [{
>  "error": {
>   "errors": [
>{
> "domain": "global",
> "reason": "required",
> "message": "You must provide at least one source component."
>}
>   ],
>   "code": 400,
>   "message": "You must provide at least one source component."
>  }
> }
> ]



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Created] (JCLOUDS-1327) putBlob with zero length Inputstream is failing on google-cloud-storage

[Chaithanya Ganta (JIRA)]

Chaithanya Ganta created JCLOUDS-1327:
-

 Summary: putBlob with zero length Inputstream is failing on 
google-cloud-storage
 Key: JCLOUDS-1327
 URL: https://issues.apache.org/jira/browse/JCLOUDS-1327
 Project: jclouds
  Issue Type: Bug
  Components: jclouds-blobstore
Affects Versions: 2.0.2
Reporter: Chaithanya Ganta


putBlob with zero length Inputstream is failing on google-cloud-storage with

org.jclouds.http.HttpResponseException: command: POST 
https://www.googleapis.com/storage/v1/b/gaul-blobstore3/o/multipart-upload/compose
 HTTP/1.1 failed with response: HTTP/1.1 400 Bad Request; content: [{
 "error": {
  "errors": [
   {
"domain": "global",
"reason": "required",
"message": "You must provide at least one source component."
   }
  ],
  "code": 400,
  "message": "You must provide at least one source component."
 }
}
]





--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

@ChaithanyaGK pushed 1 commit.

dad7d0a  Code review changes


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1120/files/22692e66b2ddc8adec4f55a74cbe9f1f987da28c..dad7d0a8f95829b8b96e54e1a3337f4cf7d585e3

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

@andrewgaul Could you please let me know is there anything else that needs to 
be addressed here.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-317779896

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

> You should add a test for putting a zero-length blob to 
> BaseBlobStoreIntegrationTest and test this against a few providers. We 
> probably want a second test for streaming zero-length InputStream payloads as 
> well.

@andrewgaul I've added integration tests for zero-length `String`, `ByteSource` 
and `InputStream` payloads,  and successfully tested against Azure blob and 
aws-s3

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-316718635

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

@ChaithanyaGK pushed 1 commit.

22692e6  Code cleanup and added integration tests


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1120/files/467eeccb71b1e041301b0b4d720c6b22ec37a401..22692e66b2ddc8adec4f55a74cbe9f1f987da28c

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

ChaithanyaGK commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap 
> headers, Headers header,
   return parts.build();
}
 
+   private static GeneratedHttpRequest 
stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+  boolean isBodyEmpty = false;
+  if (request.getPayload() != null) {
+ Long length = 
request.getPayload().getContentMetadata().getContentLength();
+ if (length != null && length == 0) {
+isBodyEmpty = true;
+ }
+  } else {
+ isBodyEmpty = true;
+  }

Thanks for the analysis @andrewgaul Will stick to the original code.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128438490

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

> could you explain the motivation for this change? I don't understand what 
> this addresses except optimizing the rare case of a zero-length PUT.

@andrewgaul  Every time I initiate a multipart upload to azure using S3Proxy it 
creates an [empty 
blob](https://github.com/andrewgaul/s3proxy/blob/master/src/main/java/org/gaul/s3proxy/S3ProxyHandler.java#L1905),
 where I've encountered this issue.

> Please open an JIRA issue and include the issue in the commit.

Updated the PR with JIRA issue-id

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-316614770

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

ChaithanyaGK commented on this pull request.



> @@ -73,7 +74,7 @@ public void testZeroLengthPutHasContentLengthHeader() 
> throws IOException, Interr
   RecordedRequest request = server.takeRequest();
   assertEquals(request.getRequestLine(), "PUT /bucket/object HTTP/1.1");
   assertEquals(request.getHeaders(CONTENT_LENGTH), ImmutableList.of("0"));
-  assertEquals(request.getHeaders(EXPECT), 
ImmutableList.of("100-continue"));
+  assertThat(request.getHeaders(EXPECT).isEmpty());

Good spot,  will update.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128437618

[jira] [Created] (JCLOUDS-1322) Zero length putBlob is making two network calls

[Chaithanya Ganta (JIRA)]

Chaithanya Ganta created JCLOUDS-1322:
-

 Summary: Zero length putBlob is making two network calls 
 Key: JCLOUDS-1322
 URL: https://issues.apache.org/jira/browse/JCLOUDS-1322
 Project: jclouds
  Issue Type: Bug
  Components: jclouds-core
 Environment: Tested using Azure
Reporter: Chaithanya Ganta
 Attachments: Charles1.png

Zero length putBlob is making two (duplicate) network calls instead of one.
This issue can be easily replicated by invoking putBlob operation with the 
zero-length blob.

ByteSource payload = ByteSource.empty();
Blob blob = blobStore.blobBuilder(blobName)
.payload(payload)
.contentLength(payload.size())
.build();
blobStore.putBlob(containerName, blob);

Attached the charles log screenshot which depicts the issue



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

ChaithanyaGK commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap 
> headers, Headers header,
   return parts.build();
}
 
+   private static GeneratedHttpRequest 
stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+  boolean isBodyEmpty = false;
+  if (request.getPayload() != null) {
+ Long length = 
request.getPayload().getContentMetadata().getContentLength();
+ if (length != null && length == 0) {
+isBodyEmpty = true;
+ }
+  } else {
+ isBodyEmpty = true;
+  }

@nacx I've debugged this further and below are my findings:

1. `content-length` is always set for non-stream payloads like 
[String](https://github.com/jclouds/jclouds/blob/0bc935dd57dc8009731d05c533edd831c8642664/core/src/main/java/org/jclouds/io/payloads/StringPayload.java#L38),
  
[ByteArray](https://github.com/jclouds/jclouds/blob/4bbca9edf943852ce1ea5aa579fa0554f770a3ea/core/src/main/java/org/jclouds/io/payloads/ByteArrayPayload.java#L31)
 etc.,
2. The only scenario I'm worried is in the case of `chunked` transfer with 
Stream payloads (like 
[InputStream](https://github.com/jclouds/jclouds/blob/4bbca9edf943852ce1ea5aa579fa0554f770a3ea/core/src/main/java/org/jclouds/io/payloads/InputStreamPayload.java#L23),
 [ByteSource 
](https://github.com/jclouds/jclouds/blob/4bbca9edf943852ce1ea5aa579fa0554f770a3ea/core/src/main/java/org/jclouds/io/payloads/ByteSourcePayload.java#L30)etc.,
 ) how to check the body is empty?

Just to verify the case, I tried to do azure putBlob without setting the 
content-length for ByteSource payload:

```
ByteSource payload = ByteSource.wrap("testdata".getBytes(Charsets.UTF_8));
Blob blob = blobStore.blobBuilder(blobName)
.payload(payload)
//.contentLength(payload.size())
.build();
blobStore.putBlob(containerName, blob);
```

I'm getting the below error saying I've to specify the content-length (which is 
not the case with non-stream payloads):
```
Exception in thread "main" java.lang.IllegalArgumentException: size must be set
at 
com.google.common.base.Preconditions.checkArgument(Preconditions.java:125)
at 
org.jclouds.azureblob.binders.BindAzureBlobMetadataToRequest.bindToRequest(BindAzureBlobMetadataToRequest.java:56)
at 
org.jclouds.rest.internal.RestAnnotationProcessor.decorateRequest(RestAnnotationProcessor.java:660)
at 
org.jclouds.rest.internal.RestAnnotationProcessor.apply(RestAnnotationProcessor.java:354)
at 
org.jclouds.rest.internal.RestAnnotationProcessor.apply(RestAnnotationProcessor.java:137)
```
So, I'm wondering whether there would be a scenario where content-length is not 
set ?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128252872

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

ChaithanyaGK commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap 
> headers, Headers header,
   return parts.build();
}
 
+   private static GeneratedHttpRequest 
stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+  boolean isBodyEmpty = false;
+  if (request.getPayload() != null) {
+ Long length = 
request.getPayload().getContentMetadata().getContentLength();
+ if (length != null && length == 0) {
+isBodyEmpty = true;
+ }
+  } else {
+ isBodyEmpty = true;
+  }

@nacx jclouds don't set the content length in case of `chunked` transfer, that 
is the reason I'm not considering the body to be empty if content length is not 
set. Please check [this 
](https://github.com/jclouds/jclouds/blob/4bbca9edf943852ce1ea5aa579fa0554f770a3ea/core/src/main/java/org/jclouds/http/HttpUtils.java#L242)


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128180992

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

ChaithanyaGK commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap 
> headers, Headers header,
   return parts.build();
}
 
+   private static GeneratedHttpRequest 
stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+  boolean isBodyEmpty = false;
+  if (request.getPayload() != null) {
+ Long length = 
request.getPayload().getContentMetadata().getContentLength();
+ if (length != null && length == 0) {
+isBodyEmpty = true;
+ }
+  } else {
+ isBodyEmpty = true;
+  }

Thanks @nacx , will update the PR.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128164859

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

@andrewgaul Please have a look at the Charles screenshot attached, where two 
calls are made in case of azure putBlob operation with zero length blob.

![charles1](https://user-images.githubusercontent.com/28896513/28322642-3d1b2142-6bf4-11e7-856e-1d3c00d6d9e2.png)

I've added a unit test case to check whether Expect header is stripped in the 
case of the empty request body.  Not sure how to add an integration test to 
actually check that only one network call is being made. Any leads on this 
would be very helpful. 


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-316083949

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

ChaithanyaGK commented on this pull request.



> @@ -73,7 +73,7 @@ public void testZeroLengthPutHasContentLengthHeader() 
> throws IOException, Interr
   RecordedRequest request = server.takeRequest();
   assertEquals(request.getRequestLine(), "PUT /bucket/object HTTP/1.1");
   assertEquals(request.getHeaders(CONTENT_LENGTH), ImmutableList.of("0"));
-  assertEquals(request.getHeaders(EXPECT), 
ImmutableList.of("100-continue"));
+  assert request.getHeaders(EXPECT).isEmpty();

Done

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r127988452

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

[Chaithanya Ganta]

ChaithanyaGK commented on this pull request.



> @@ -357,6 +357,22 @@ public GeneratedHttpRequest apply(Invocation invocation) 
> {
   if (request.getPayload() != null) {
  
contentMetadataCodec.fromHeaders(request.getPayload().getContentMetadata(), 
headers);
   }
+
+  boolean isEmptyBody = false;
+  if ("POST".equals(requestMethod) || "PUT".equals(requestMethod)) {
+ if (request.getPayload() != null) {
+Long length = 
request.getPayload().getContentMetadata().getContentLength();
+if (length != null && length == 0) {
+   isEmptyBody = true;
+}
+ } else {
+isEmptyBody = true;
+ }
+  }
+  if (isEmptyBody) {
+ request = request.toBuilder().removeHeader("Expect").build();
+  }

@nacx Refactored the code to a separate method and added a unit test case to 
check the functionality. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r127988381

Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)

[Chaithanya Ganta]

@ChaithanyaGK pushed 1 commit.

467eecc  Refactored the code and added a unit test


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1120/files/e12e21821dd3c54bdbae0954216d7d0a1f8514d9..467eeccb71b1e041301b0b4d720c6b22ec37a401

Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)

[Chaithanya Ganta]

ChaithanyaGK commented on this pull request.



> @@ -357,6 +357,22 @@ public GeneratedHttpRequest apply(Invocation invocation) 
> {
   if (request.getPayload() != null) {
  
contentMetadataCodec.fromHeaders(request.getPayload().getContentMetadata(), 
headers);
   }
+
+  boolean isEmptyBody = false;
+  if ("POST".equals(requestMethod) || "PUT".equals(requestMethod)) {

@nacx Mostly, I came across Expect header being set for either POST or PUT 
request. But, I couldn't find any mention of that Expect header is restricted 
to some Http methods. I think we can remove this check altogether. WDYT ?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r127745941

[jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)

[Chaithanya Ganta]

Without request body, there's no point in asking for 100-continue.

With 
[JavaUrlHttpCommandExecutor](https://github.com/jclouds/jclouds/blob/f3c3f3b30620ad62dc502cf79bf121ec1773396e/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java),
 HttpUrlConnection is throwing an 
[IOException](https://github.com/jclouds/jclouds/blob/f3c3f3b30620ad62dc502cf79bf121ec1773396e/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java#L96)
 if we expect 100-continue for an PUT/POST request with empty body although it 
is returning 201 response code, it is resulting in another network call when we 
do 
[getHeaderFields](https://github.com/jclouds/jclouds/blob/f3c3f3b30620ad62dc502cf79bf121ec1773396e/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java#L113)
 on the connection.

This issue can be easily replicated by invoking putBlob operation with 
zero-length blob.
```
ByteSource payload = ByteSource.empty();
Blob blob = blobStore.blobBuilder(blobName)
.payload(payload)
.contentLength(payload.size())
.build();
blobStore.putBlob(containerName, blob);
```
You can view, comment on, or merge this pull request online at:

  https://github.com/jclouds/jclouds/pull/1120

-- Commit Summary --

  * Remove Expect header for PUT and POST reqs with content-length=0

-- File Changes --

M apis/s3/src/test/java/org/jclouds/s3/S3ClientMockTest.java (2)
M core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java 
(16)

-- Patch Links --

https://github.com/jclouds/jclouds/pull/1120.patch
https://github.com/jclouds/jclouds/pull/1120.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120