[BEAM-978] Support bulk get file size in GcsUtil. #1359

peihe · 2016-11-15T02:29:42Z

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify. (Even better, enable
Travis-CI on your fork and ensure the whole test matrix passes).
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

peihe · 2016-11-15T02:30:49Z

R: @kennknowles @tgroh

kennknowles

Failures are checkstyle, FYI.

kennknowles · 2016-11-15T05:18:43Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/GcsUtil.java

-        }
-        throw new IOException("Unable to get file size", e);
-     }
+  List<Long> fileSize(Collection<GcsPath> paths) throws IOException {


Readability nit: I would prefer fileSizes. At call sites, this makes the meaning of the collection more clear.

I would prefer sizes; is files not implicit here?

Updated to fileSizes:

I feel there could be ambiguity for "GcsUtil.sizes(paths)".

kennknowles · 2016-11-15T05:21:52Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/GcsUtil.java

@@ -483,12 +475,39 @@ public Void call() throws IOException {
      Thread.currentThread().interrupt();
      throw new IOException("Interrupted while executing batch GCS request", e);
    } catch (ExecutionException e) {
+      if (e.getCause() instanceof FileNotFoundException) {


Is there a way to get all the exceptions from failed requests? Are they already contained in this one?

Currently, e will be the first exception from all Futures.

I am going to propose a bulk get metadata API. And, with Metadata, we can wait for all requests to finish, and store the exception in Metadata if any request fails.

kennknowles · 2016-11-15T05:23:43Z

sdks/java/core/src/test/java/org/apache/beam/sdk/util/GcsUtilTest.java

-    MockLowLevelHttpResponse notFoundResponse = new MockLowLevelHttpResponse();
-    notFoundResponse.setContent("");
-    notFoundResponse.setStatusCode(HttpStatusCodes.STATUS_CODE_NOT_FOUND);
+    String content = "Content-Type: application/http\n"


This test seems overspecified. It should not require any of the details, just the HTTP 404 error code and perhaps the nested {error:...} bit.

kennknowles · 2016-11-15T05:24:57Z

sdks/java/core/src/test/java/org/apache/beam/sdk/util/GcsUtilTest.java

+        + " \"generation\": \"1470763581808000\",\n"
+        + " \"metageneration\": \"2\",\n"
+        + " \"contentType\": \"application/txt\",\n"
+        + " \"timeCreated\": \"2016-08-09T17:26:21.801Z\",\n"


This is very overspecified. Anything that could change between requests, such as creation time, etc, should not be included.

It seems as though we should be able to generate a response of the type we desire instead of dumping in a response string. Is that not the case?

kennknowles · 2016-11-15T05:28:06Z

sdks/java/core/src/test/java/org/apache/beam/sdk/util/GcsUtilTest.java

+      String content) throws Exception {
+    MockLowLevelHttpResponse notFoundResponse = new MockLowLevelHttpResponse()
+        .setContentType("multipart/mixed; boundary=batch_foobarbaz")
+        .setContent(content)


Here, where you mock the low level response, I think you should either omit or fill in junk for pieces that GcsUtil should not depend on. But you shouldn't mock the low level response unless our code relies on it. Instead, mock the storageClient.

tgroh · 2016-11-16T01:08:51Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/GcsUtil.java

@@ -266,16 +266,16 @@ Integer getUploadBufferSizeBytes() {
   * Returns the file size from GCS or throws {@link FileNotFoundException}
   * if the resource does not exist.
   */
-  public long fileSize(GcsPath path) throws IOException {
-    return fileSize(ImmutableList.of(path)).get(0);
+  public long fileSizes(GcsPath path) throws IOException {


this should still be fileSize

peihe

I would like to defer improving the bulk get fileSizes together with https://issues.apache.org/jira/browse/BEAM-989

Since copy() and remove() need similar tests, I will just do them together.
I propose to merge this PR. And, I will send another one to fix all tests. Otherwise, I can append a commit maybe few days later.

peihe · 2016-11-15T20:12:32Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/GcsUtil.java

-        }
-        throw new IOException("Unable to get file size", e);
-     }
+  List<Long> fileSize(Collection<GcsPath> paths) throws IOException {


Updated to fileSizes:

I feel there could be ambiguity for "GcsUtil.sizes(paths)".

peihe · 2016-11-15T23:16:05Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/GcsUtil.java

@@ -483,12 +475,39 @@ public Void call() throws IOException {
      Thread.currentThread().interrupt();
      throw new IOException("Interrupted while executing batch GCS request", e);
    } catch (ExecutionException e) {
+      if (e.getCause() instanceof FileNotFoundException) {


Currently, e will be the first exception from all Futures.

I am going to propose a bulk get metadata API. And, with Metadata, we can wait for all requests to finish, and store the exception in Metadata if any request fails.

asfbot · 2016-11-19T02:04:49Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_Build/4/
--none--

peihe · 2016-11-19T02:10:48Z

@kennknowles @tgroh

I need mockito 2 to mock the final class BatchRequest.
However, mockito upgrading is blocked by a bytes-buddy versions conflict.
I think it will take us for a while to get the fix.

So, I suggest to move forward with this PR and address https://issues.apache.org/jira/browse/BEAM-989 later on.

thoughts?

kennknowles · 2016-11-21T18:26:53Z

Our use of bytebuddy should be entirely private. Can you please file a JIRA to shade it? Unfortunately, we have hit blockers in getting the SDK's private dependencies shaded by default, so we need to still just aggressively shade things right away when they cause any trouble.

peihe · 2016-11-21T22:00:40Z

Filed https://issues.apache.org/jira/browse/BEAM-1019

More issues came out when updating to mockito 2. (Looks like there are few tests depends on the internal details of mock classes.)

I am not going to jump into the rabbit hole right now, will address them one by one.

kennknowles · 2016-11-28T21:05:17Z

Following up on my last comment: good shading by Mockito would help us, but our own shading cannot help. But we can probably upgrade bytebuddy without too much difficulty, despite backwards-incompatible changes between 1.4.x and 1.5.x

kennknowles · 2016-11-30T22:13:31Z

sdks/java/core/src/test/java/org/apache/beam/sdk/util/GcsUtilTest.java

+        + " }\n"
+        + "}\n"
+        + "\n"
+        + "--batch_CnzAnRit7rQ_AAax2S5XfdM--\n";


This boundary should be in a constant, and match below.

kennknowles · 2016-11-30T22:14:43Z

sdks/java/core/src/test/java/org/apache/beam/sdk/util/GcsUtilTest.java

+        + "HTTP/1.1 200 OK\n"
+        + "Content-Length: 708\n"
+        + "\n"
+        + "{\n"


Does there exist a Java API to create an object that will dump this JSON, so you don't have to know and maintain the exact format?

kennknowles · 2016-11-30T22:16:22Z

I'm OK with moving on with this since PRs to clean it up are already in place.

asfbot · 2016-12-05T21:20:30Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5529/
--none--

peihe · 2016-12-05T22:00:49Z

PTAL

I removed the test that uses long fragile string, but kept the shorter one.
I think the test coverage is enough for current state (better than current code). I will improve the tests with mockito 2 with a separate PR.

kennknowles requested changes Nov 15, 2016

View reviewed changes

tgroh reviewed Nov 16, 2016

View reviewed changes

peihe force-pushed the bulk-size branch 2 times, most recently from c95aa4e to 57b8c74 Compare November 16, 2016 04:16

peihe commented Nov 16, 2016

View reviewed changes

kennknowles reviewed Nov 30, 2016

View reviewed changes

peihe added 4 commits December 5, 2016 10:30

[BEAM-978] Support bulk get file size in GcsUtil.

75d24ca

fixup! address comments.

232374f

fixup! checkstyles.

4a89684

fixup! address comments.

2c374f2

peihe force-pushed the bulk-size branch from f6586af to 2c374f2 Compare December 5, 2016 20:47

asfgit closed this in 409b5df Dec 8, 2016

peihe mentioned this pull request Dec 14, 2016

[BEAM-1153] GcsUtil: use non-batch API for single file size requests. #1611

Closed

4 tasks

peihe deleted the bulk-size branch August 15, 2017 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-978] Support bulk get file size in GcsUtil. #1359

[BEAM-978] Support bulk get file size in GcsUtil. #1359

peihe commented Nov 15, 2016

peihe commented Nov 15, 2016

kennknowles left a comment

kennknowles Nov 15, 2016

tgroh Nov 15, 2016

peihe Nov 15, 2016

kennknowles Nov 15, 2016

peihe Nov 15, 2016

kennknowles Nov 15, 2016

kennknowles Nov 15, 2016

tgroh Nov 15, 2016

kennknowles Nov 15, 2016

tgroh Nov 16, 2016

peihe left a comment

peihe Nov 15, 2016

peihe Nov 15, 2016

asfbot commented Nov 19, 2016

peihe commented Nov 19, 2016

kennknowles commented Nov 21, 2016

peihe commented Nov 21, 2016

kennknowles commented Nov 28, 2016

kennknowles Nov 30, 2016

kennknowles Nov 30, 2016

kennknowles commented Nov 30, 2016

asfbot commented Dec 5, 2016

peihe commented Dec 5, 2016

[BEAM-978] Support bulk get file size in GcsUtil. #1359

[BEAM-978] Support bulk get file size in GcsUtil. #1359

Conversation

peihe commented Nov 15, 2016

peihe commented Nov 15, 2016

kennknowles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peihe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfbot commented Nov 19, 2016

peihe commented Nov 19, 2016

kennknowles commented Nov 21, 2016

peihe commented Nov 21, 2016

kennknowles commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennknowles commented Nov 30, 2016

asfbot commented Dec 5, 2016

peihe commented Dec 5, 2016