[BEAM-747] Improve FileChecksumMatcher That Inconsistent With Filesystem #1189

markflyhigh · 2016-10-25T21:46:15Z

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.

Add retry in FileChecksumMatcher when following conditions happens:

IOException raised from filesystem
No file found from output directory
number of files found from fs doesn't equal to expected number, which is parsed from shard name using a name template. Default template "SSS-of-NNN" will be used when no template is specified.

Default retry times are 4. Default sleep duration between each retry are 10s.

markflyhigh · 2016-10-26T00:21:07Z

+R: @jasonkuster

jasonkuster · 2016-11-07T09:28:00Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

  private String actualChecksum;

  public FileChecksumMatcher(String checksum, String filePath) {
+    this(checksum, filePath, null);


Instead of passing in null here, probably clearer to pass in DEFAULT_SHARD_TEMPLATE. Leave the null-handling in the constructor below of course.

jasonkuster · 2016-11-07T09:29:58Z

sdks/java/core/src/test/java/org/apache/beam/sdk/testing/FileChecksumMatcherTest.java

            IOChannelUtils.resolve(tmpFolder.getRoot().getPath(), "*"));

    assertThat(pResult, matcher);
  }
+
+  @Test
+  public void testReadWithRetriesFailsWhenTemplateIncorrect()


You have a test here that verifies incorrect template behavior, but no test which verifies any template other than the default template. Probably a good idea to add one?

jasonkuster · 2016-11-07T09:30:58Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

+
+  /**
+   * Check if total number of files is correct by comparing with the number that
+   * is parsed from shard name using a name template. If no template are specified,


If no template is specified

markflyhigh · 2016-11-08T01:27:09Z

PTAL @jasonkuster

markflyhigh · 2016-11-09T00:42:18Z

@kennknowles

kennknowles · 2016-11-10T22:15:00Z

@markflyhigh can you rebase to kick the tests until we get a clean execution? (or until you uncover a real bug)

markflyhigh · 2016-11-14T22:00:51Z

Rebased with latest master and all integration tests passed. The Jenkins build failed because known existing bugs that doesn't related to this PR.

PTAL: @kennknowles @jasonkuster

jasonkuster · 2016-11-21T17:38:15Z

LGTM, assuming Jenkins failure is transient. Suggest rebasing and kicking tests off again.

asfbot · 2016-11-21T17:43:03Z

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

markflyhigh · 2016-11-28T19:04:12Z

friendly ping @kennknowles

kennknowles · 2016-11-28T20:41:17Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

+   */
+  @VisibleForTesting
+  boolean checkTotalNumOfFiles(Collection<String> files) {
+    for (String filePath : files.toArray(new String[0])) {


Why do you need toArray here? Should be able to just use for (String filePath : files) { ... }. If not, please comment.

right, should just use the simple way to iterate collections. done

kennknowles · 2016-11-28T20:50:18Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

+          .withInitialBackoff(DEFAULT_SLEEP_DURATION)
+          .withMaxRetries(MAX_READ_RETRIES);
+
+  private static final String DEFAULT_SHARD_TEMPLATE = "\\S*\\d+-of-(\\d+)$";


This should have type Pattern unless there is a special reason not to compile it right away.

kennknowles · 2016-11-28T20:51:50Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

+          .withInitialBackoff(DEFAULT_SLEEP_DURATION)
+          .withMaxRetries(MAX_READ_RETRIES);
+
+  private static final String DEFAULT_SHARD_TEMPLATE = "\\S*\\d+-of-(\\d+)$";


I recommend using named groups and allowing space via (?x) for greater clarity. Here I've just demonstrated adding also a capture for shardnum.

// Whitespace is ignored Pattern.compile("(?x) \\S* (?shardnum \\d+) -of- (?numshards \\d+)")

This will allow your later use of .group(1) to be .group("numshards").

thanks! done

kennknowles · 2016-11-28T20:52:07Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

+    this(checksum, filePath, DEFAULT_SHARD_TEMPLATE);
+  }
+
+  public FileChecksumMatcher(String checksum, String filePath, String shardTemplate) {


Should take a Pattern unless there is a special reason not to.

kennknowles · 2016-11-28T20:52:55Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

@@ -66,48 +99,105 @@ public FileChecksumMatcher(String checksum, String filePath) {

    this.expectedChecksum = checksum;
    this.filePath = filePath;
+    this.shardTemplate =
+        Pattern.compile(shardTemplate == null ? DEFAULT_SHARD_TEMPLATE : shardTemplate);


This shouldn't check for null but should require the user to call the other constructor.

I'll add a precondition check to make sure it's non-null.

kennknowles · 2016-11-28T20:53:27Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

-      LOG.info("Generated checksum for output data: {}", actualChecksum);
+    // Verify outputs. Checksum is computed using SHA-1 algorithm
+    actualChecksum = computeHash(outputs);
+    LOG.info("Generated checksum: {}", actualChecksum);


Silence this or LOG.debug. If there is a mismatch the error thrown should contain this information.

kennknowles · 2016-11-28T20:54:25Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

        LOG.info(
-            "[{} of {}] Read {} lines from file: {}", i, files.size() - 1, lines.size(), file);
+            "[{} of {}] Read {} lines from file: {}", i, files.size(), lines.size(), file);


Silence, or LOG.debug.

kennknowles · 2016-11-28T20:54:50Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

+      try {
+        // Match inputPath which may contains glob
+        Collection<String> files = factory.match(filePath);
+        LOG.info("Found {} file(s) by matching the path: {}", files.size(), filePath);


Silence or LOG.debug

kennknowles · 2016-11-28T20:56:18Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

+      // once match, extract total number of shards and compare to file list
+      return files.size() == Integer.parseInt(matcher.group(1));
+    }
+    LOG.warn("No name matches the shard template: {}", shardTemplate);


I believe this should either be an error or remain silent. When is this expected to be OK and when is it not OK?

kennknowles · 2016-11-28T20:56:47Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/FileChecksumMatcher.java

+        continue;
+      }
+      // once match, extract total number of shards and compare to file list
+      return files.size() == Integer.parseInt(matcher.group(1));


See above how to make this more self-explanatory as .group("numshards"). Either way, requires a brief comment about the expectations of the shard template.

thanks! I'll add javadoc to explain more for customized shard template.

markflyhigh · 2016-11-29T22:53:41Z

PTAL @kennknowles

kennknowles

Nice. I'll merge. Thanks!

markflyhigh force-pushed the file-matcher-read-retry branch from bc6a892 to be73409 Compare October 25, 2016 21:57

jasonkuster requested changes Nov 7, 2016

View reviewed changes

markflyhigh force-pushed the file-matcher-read-retry branch from be73409 to e3cb725 Compare November 7, 2016 23:42

markflyhigh changed the title ~~[BEAM-747] Fix FileChecksumMatcher That Inconsistent With Filesystem~~ [BEAM-747] Improve FileChecksumMatcher That Inconsistent With Filesystem Nov 9, 2016

markflyhigh force-pushed the file-matcher-read-retry branch 2 times, most recently from 5385c10 to 65a09aa Compare November 14, 2016 19:27

markflyhigh force-pushed the file-matcher-read-retry branch from 65a09aa to b29d495 Compare November 22, 2016 23:10

kennknowles requested changes Nov 28, 2016

View reviewed changes

Mark Liu added 2 commits November 29, 2016 11:39

[BEAM-747] Fix FileChecksumMatcher That Inconsistent With FS

fac751c

fixup! Add More Unit Tests

5f26347

markflyhigh force-pushed the file-matcher-read-retry branch from b29d495 to 7adbe6c Compare November 29, 2016 20:40

fixup! Improve shard template and javadoc

ae339c1

markflyhigh force-pushed the file-matcher-read-retry branch from 7adbe6c to ae339c1 Compare November 29, 2016 22:27

kennknowles approved these changes Nov 30, 2016

View reviewed changes

asfgit closed this in b75a764 Nov 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-747] Improve FileChecksumMatcher That Inconsistent With Filesystem #1189

[BEAM-747] Improve FileChecksumMatcher That Inconsistent With Filesystem #1189

markflyhigh commented Oct 25, 2016 •

edited

markflyhigh commented Oct 26, 2016

jasonkuster Nov 7, 2016

markflyhigh Nov 7, 2016

jasonkuster Nov 7, 2016

markflyhigh Nov 7, 2016

jasonkuster Nov 7, 2016

markflyhigh commented Nov 8, 2016

markflyhigh commented Nov 9, 2016

kennknowles commented Nov 10, 2016

markflyhigh commented Nov 14, 2016

jasonkuster commented Nov 21, 2016

asfbot commented Nov 21, 2016

markflyhigh commented Nov 28, 2016

kennknowles Nov 28, 2016

markflyhigh Nov 29, 2016

kennknowles Nov 28, 2016

markflyhigh Nov 29, 2016

kennknowles Nov 28, 2016

markflyhigh Nov 29, 2016

kennknowles Nov 28, 2016

markflyhigh Nov 29, 2016

kennknowles Nov 28, 2016

markflyhigh Nov 29, 2016

kennknowles Nov 28, 2016

markflyhigh Nov 29, 2016

kennknowles Nov 28, 2016

kennknowles Nov 28, 2016

kennknowles Nov 28, 2016

kennknowles Nov 28, 2016

markflyhigh Nov 29, 2016

markflyhigh commented Nov 29, 2016

kennknowles left a comment

[BEAM-747] Improve FileChecksumMatcher That Inconsistent With Filesystem #1189

[BEAM-747] Improve FileChecksumMatcher That Inconsistent With Filesystem #1189

Conversation

markflyhigh commented Oct 25, 2016 • edited

markflyhigh commented Oct 26, 2016

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

markflyhigh commented Nov 8, 2016

markflyhigh commented Nov 9, 2016

kennknowles commented Nov 10, 2016

markflyhigh commented Nov 14, 2016

jasonkuster commented Nov 21, 2016

asfbot commented Nov 21, 2016

markflyhigh commented Nov 28, 2016

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

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

markflyhigh commented Nov 29, 2016

kennknowles left a comment

Choose a reason for hiding this comment

markflyhigh commented Oct 25, 2016 •

edited