[BEAM-2963] Preliminary refactors to PackageUtil by kennknowles · Pull Request #3920 · apache/beam

kennknowles · 2017-09-29T05:11:02Z

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

These are some basic refactors to give PackageUtil a little TLC, making it more usable and re-usable.

kennknowles · 2017-09-29T05:12:51Z

R: @tgroh since I know you are looking at artifact-related things. This may or may not actually be interesting to you.

It is probably easiest to review one commit at a time.

kennknowles · 2017-09-30T16:29:34Z

The tests all passed, then timeout occurred while building Python. I've bumped the timeout and will retest, but FYI this is g2g.

kennknowles · 2017-10-02T16:15:35Z

run precommit pipeline java

kennknowles · 2017-10-02T16:16:24Z

retest this please

tgroh · 2017-10-02T20:02:28Z

...gle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/PackageUtil.java

+    }
+  }
+
+  private StagingResult stagePackageBlithely(


While I really really like this name, I fear that it doesn't communicate what it does sufficiently precisely, given standard java jargon

tgroh · 2017-10-02T20:06:11Z

...gle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/PackageUtil.java

+          throw e;
+        } else {
+          LOG.warn(
+              "Upload attempt failed, sleeping before retrying staging of classpath: {}",


'staging of package'

tgroh · 2017-10-02T20:08:22Z

...gle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/PackageUtil.java

-        }
-      }));
+    for (String classpathElement : classpathElements) {
+      DataflowPackage sourcePackage = new DataflowPackage();


Is the creation + value migration worth factoring into its own method?

tgroh · 2017-10-02T20:11:39Z

...gle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/PackageUtil.java

+    }
+  }
+
+  private StagingResult stagePackageBlithely(


While I really like this name, I don't think it communicates why this is so blithe in standard java Jargon.

Suggestions? I settled on this suffix some while ago for lack of a better option.

"Crashily"?

kennknowles

PTAL

kennknowles · 2017-10-04T22:08:09Z

...gle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/PackageUtil.java

+          throw e;
+        } else {
+          LOG.warn(
+              "Upload attempt failed, sleeping before retrying staging of classpath: {}",


tgroh wrote:
'staging of package'

Done.

kennknowles · 2017-10-04T22:08:09Z

...gle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/PackageUtil.java

-        }
-      }));
+    for (String classpathElement : classpathElements) {
+      DataflowPackage sourcePackage = new DataflowPackage();


tgroh wrote:
Is the creation + value migration worth factoring into its own method?

TBH this should be moved out to the command line parsing. It has no place here. For now I'd rather leave it as-is (this bit is a code move)

kennknowles · 2017-10-04T22:08:09Z

...gle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/PackageUtil.java

+    }
+  }
+
+  private StagingResult stagePackageBlithely(


kennknowles wrote:
"Crashily"?

Done. Split into tryStagePackage which is trivial straight-line code, tryStagePackageWithRetries that does just that loop, crashing if it hits something unrecoverable, and stagePackageSynchronously that just decorates the error message.

coveralls · 2017-10-05T00:56:06Z

Coverage increased (+0.02%) to 69.588% when pulling f630545 on kennknowles:PackageUtil into 7db0f13 on apache:master.

kennknowles · 2017-10-05T19:56:00Z

run dataflow validatesrunner

kennknowles · 2017-10-05T20:40:55Z

We've seen a couple jobs run, since the basic presubmit does that, but I'll wait to see a lot of jobs run, since if this breaks it is very bad.

kennknowles · 2017-10-06T00:23:27Z

All the jobs in the ValidatesRunner suite succeeded. It timed out in dependency analysis. Filed BEAM-3024.

Refactor PackageUtil for more and simpler asynchrony Use AutoValue for Dataflow PackageAttributes Make PackageUtil a proper class encapsulating its ExecutorService

coveralls · 2017-10-06T01:58:24Z

Coverage increased (+0.07%) to 69.604% when pulling 58b6453 on kennknowles:PackageUtil into 31da49c on apache:master.

kennknowles changed the title ~~Package util~~ [BEAM-2963] Preliminary refactors to PackageUtil Sep 29, 2017

kennknowles force-pushed the PackageUtil branch 3 times, most recently from 4b30b9c to 2eb6794 Compare September 30, 2017 13:56

tgroh reviewed Oct 2, 2017

View reviewed changes

kennknowles commented Oct 4, 2017

View reviewed changes

tgroh approved these changes Oct 5, 2017

View reviewed changes

kennknowles added 3 commits October 5, 2017 17:35

Make PackageUtil a proper class encapsulating its ExecutorService

a211bd9

Use AutoValue for Dataflow PackageAttributes

a328127

Refactor PackageUtil for more and simpler asynchrony

58b6453

kennknowles force-pushed the PackageUtil branch from f630545 to 58b6453 Compare October 6, 2017 00:35

asfgit merged commit 58b6453 into apache:master Oct 6, 2017

kennknowles deleted the PackageUtil branch April 25, 2018 02:10

Conversation

kennknowles commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennknowles commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennknowles commented Sep 30, 2017

Uh oh!

kennknowles commented Oct 2, 2017

Uh oh!

kennknowles commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennknowles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 5, 2017

Uh oh!

kennknowles commented Oct 5, 2017

Uh oh!

kennknowles commented Oct 5, 2017

Uh oh!

kennknowles commented Oct 6, 2017

Uh oh!

coveralls commented Oct 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kennknowles commented Sep 29, 2017 •

edited

Loading

kennknowles commented Sep 29, 2017 •

edited

Loading