added method to job to get PromiseValue using handle #14

billy1380 · 2015-01-30T20:11:17Z

No description provided.

aozarov · 2015-02-12T17:56:07Z

java/src/main/java/com/google/appengine/tools/pipeline/Job.java

@@ -385,6 +386,15 @@ private void setCurrentRunGuid(String guid) {
    updateSpec.getNonTransactionalGroup().includeSlot(promisedValue.getSlot());
    return promisedValue;
  }
+
+  public <F> PromisedValue<F> promise(String promiseHandle) {


javadoc.
Also, I wonder what is your use case? PromisedValue only expose its handle, so the only value that I can think
of is if you want to "pass around" the Value to deeply nested children and make their run depends on it.
I am pretty sure this is not going to work correctly if you try to use it with different root jobs (so maybe a more elegant form of passing [not by handle] that would only work between parent to child would be preferred).
In any case, I think you should provide tests for it.

My use case is described in issue #13 and you are not far off.

I have no intention of using the promised value across root jobs, so I have no problem with adding that as a restriction.

Updates soon...

billy1380 · 2015-02-13T11:24:42Z

I will try to submit an update very soon, thanks for the feedback.

…eSlot and acceptPromisedValue

aozarov · 2015-02-14T20:20:00Z

java/src/main/java/com/google/appengine/tools/pipeline/Job.java

+   * that will be provided asynchronously by some external agent. This can be
+   * used to share the same value with child {@link Job}s
+   * 
+   * 


Nit, one empty line is enough.

aozarov · 2015-02-14T20:29:17Z

Looks good. Please add tests and it should be ready for merge.

aozarov · 2015-02-14T20:38:18Z

java/src/main/java/com/google/appengine/tools/pipeline/impl/PipelineManager.java

+   * @param promiseHandle Key to find the {@link Slot}
+   * @return A {@link Slot} if the handle can be resolved otherwise <code>null</code>.
+   */
+  public static Slot getPromisedValueSlot(String promiseHandle) {


Do we really need this method? Can dealing with the exception be part of Job?
This method would mask InterruptedException.

If you are worried about masking the exception I can follow the example of acceptPromisedValue and throw NoSuchObjectException and OrphanedObjectException, but the method itself is required because queryPromisedValueSlot is private and I am not sure I should change it's scope.

Edit (note): if I make it throw the exceptions then I will probably have to throw them in promise() which does not seem to be done by any other job methods.

… return docs

promise

billy1380 · 2015-05-09T14:15:07Z

I have added unit tests and applied all the comments where possible. Please review.

aozarov · 2015-05-10T23:51:17Z

java/src/main/java/com/google/appengine/tools/pipeline/Job.java

+
+  /**
+   * Invoke this method from within the {@code run} method of a <b>generator
+   * job</b> in order to get some value that has been previously declared and


to get a Promised Value that was created by an ancestor job?

That is definitely clearer... but... this is the language that the rest of the javadoc uses.

I am fine with such inconsistency (though maybe we can phrase it in a slightly more consistent way).
Basically, I think we should make it clear that it should not be used across Jobs.

- updated javadoc language to highlight ansestor constraint for promises

billy1380 · 2015-05-11T20:27:13Z

updated @aozarov

aozarov · 2015-05-11T22:12:58Z

java/src/test/java/com/google/appengine/tools/pipeline/MiscPipelineTest.java

+      futureCall(new FillPromiseJob(), immediate("3"),
+          immediate(promise.getHandle()));
+      FutureValue<String> child4 = futureCall(new StringAdderJob(), child1,
+          child2);


Nit, I don't think the wrap is necessary here (max width 100)

aozarov · 2015-05-11T22:21:27Z

Thank you. Nice work. few minor comments and should be ready for merge.

- added wait setting to subsequent jobs in testFilledPromiseFromHandle - return boolean from PromiseFromNonExistentHandleParentJob and asserting true

…ines.git

billy1380 · 2015-05-12T08:37:42Z

@aozarov applied comments.

added method to job to get PromiseValue using handle

aozarov · 2015-05-12T16:40:51Z

Merged. Thank you!

billy1380 · 2015-05-12T16:43:09Z

https://www.youtube.com/watch?v=0wHYHMpHmSg

aozarov · 2015-07-26T20:36:08Z

Hey @billy1380.
Thank you for your quick response and your willingness to help resolving this issue.

Indeed it feels like testUnfilledPromiseFromHandle is covering the second use-case I described (passing a filled-slot that will never trigger SlotFilled and, possibly, blocked Job[s]) but
the reason this test is passing is because pipeline is, shamefully, not trying to optimize on
task-queue calls. Basically when you pass an immediate as param pipeline will treat it
as a filled slot and register a SlotField task. However when you register a FutureValue
it does not register a SlotField task as it expect that task to be triggered when the
slot get fields.

A SlotField task is the task that checks all the Jobs (run barrier) associated with a Slot
and triggers a RunJob task for every Job that all its other Slots are filled.

If you change testUnfilledPromiseFromHandle and remove the extra immediate arguments
you should be able to reproduce this issue (and the test should never complete).
Here is a modified version that you can try:

  public void testFilledPromiseFromHandle_noArgs() throws Exception {
    PipelineService service = PipelineServiceFactory.newPipelineService();
    String pipelineId = service.startNewPipeline(new FilledPromiseFromHandleParentJobNoArgs());
    String value = waitForJobToComplete(pipelineId);
    assertEquals("13", value);
  }

  @SuppressWarnings("serial")
  private static class FilledPromiseFromHandleParentJobNoArgs extends Job0<String> {

    @Override
    public Value<String> run() throws Exception {
      PromisedValue<String> promise = newPromise();
      WaitForSetting wait = waitFor(futureCall(new FillPromiseJob(), immediate("3"),
          immediate(promise.getHandle())));

      return futureCall(new PromiseDelegateJobNoArgs(), immediate(promise.getHandle()), wait);
    }
  }

  @SuppressWarnings("serial")
  private static class PromiseDelegateJobNoArgs extends Job1<String, String> {
    @Override
    public Value<String> run(String handle) {
      PromisedValue<String> promise = promise(handle);
      return futureCall(new StringAdderJobNoArgs(), promise);
    }
  }

  @SuppressWarnings("serial")
  private static class StringAdderJobNoArgs extends Job1<String, String> {
    @Override
    public Value<String> run(String value) {
      return immediate("1" + value);
    }
  }

We could change PipelineManager to treat PromisedValue similar to immediate if it was field
but then we will face the second issue (that for me is highly reproducible using the w test) where we get a race-condition between reading
a state of a Slot and updating the Slot. This issue could have never happened before
as the PromisedValue Slot binding to a Job was fixed before it could ever be filled.

In my testUnfilledPromiseFromHandle I could clearly see this race conditions (via some printouts)
where StringAdderJob is being registered with a stale copy of the PromisedValue's slot (slot is
marked as yet unfilled) but then before it is saved it gets filled by FillPromiseJob.
BTW, the saving after the JobFilled could 'revert' the filled value (and effect other dependent jobs).
One way to deal with this issue is to treat PromiseValue's slot specially and transactionally verify
it wasn't change before saving it.

aozarov · 2015-07-27T19:42:35Z

Hey @billy1380, I plan to revert this change but it should be easy to re-apply it once you have the fix.
Thank you.

billy1380 · 2015-07-27T19:44:55Z

Go for it... I have picked up the latest version before the rollback, ran both tests and can recreate the issue quite consistently.

Will keep you posted.

added method to get PromiseValue using handle

b0e27bf

billy1380 mentioned this pull request Jan 30, 2015

Create promise from handle #13

Open

aozarov reviewed Feb 12, 2015
View reviewed changes

billy1380 added 3 commits February 13, 2015 12:30

fixed constructor to call this instead of super

9b43be5

added method queryPromisedValueSlot that is used from getPromisedValu…

202d8f2

…eSlot and acceptPromisedValue

added javadoc to newPromise and promise methods

c9ad75f

aozarov reviewed Feb 14, 2015
View reviewed changes

billy1380 added 4 commits May 8, 2015 23:22

Merge remote-tracking branch 'upstream/master'

f6821af

removed double line spacing in javadoc

99db1fc

removed note about method returning null and added to the rest of the…

eba5642

… return docs

added 3 tests for unfilled promise, filled promise and non-existent

9e418c2

promise

aozarov reviewed May 10, 2015
View reviewed changes

- used java 7 syntax for declaring generic

32321eb

- updated javadoc language to highlight ansestor constraint for promises

aozarov reviewed May 11, 2015
View reviewed changes

billy1380 added 2 commits May 12, 2015 09:35

- reformatted code for 100 char line

79c02c3

- added wait setting to subsequent jobs in testFilledPromiseFromHandle - return boolean from PromiseFromNonExistentHandleParentJob and asserting true

Merge branch 'master' of https://github.com/billy1380/appengine-pipel…

7349c00

…ines.git

aozarov added a commit that referenced this pull request May 12, 2015

Merge pull request #14 from billy1380/master

574c75c

added method to job to get PromiseValue using handle

aozarov merged commit 574c75c into GoogleCloudPlatform:master May 12, 2015

soundofjw added the Java label May 12, 2015

aozarov mentioned this pull request Jul 25, 2015

Revert "added method to job to get PromiseValue using handle" #46

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added method to job to get PromiseValue using handle #14

added method to job to get PromiseValue using handle #14

billy1380 commented Jan 30, 2015

aozarov Feb 12, 2015

billy1380 Feb 13, 2015

billy1380 commented Feb 13, 2015

aozarov Feb 14, 2015

aozarov commented Feb 14, 2015

aozarov Feb 14, 2015

billy1380 May 9, 2015

billy1380 commented May 9, 2015

aozarov May 10, 2015

billy1380 May 11, 2015

aozarov May 11, 2015

billy1380 commented May 11, 2015

aozarov May 11, 2015

aozarov commented May 11, 2015

billy1380 commented May 12, 2015

aozarov commented May 12, 2015

billy1380 commented May 12, 2015

aozarov commented Jul 26, 2015

aozarov commented Jul 27, 2015

billy1380 commented Jul 27, 2015

added method to job to get PromiseValue using handle #14

added method to job to get PromiseValue using handle #14

Conversation

billy1380 commented Jan 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billy1380 commented Feb 13, 2015

Choose a reason for hiding this comment

aozarov commented Feb 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billy1380 commented May 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billy1380 commented May 11, 2015

Choose a reason for hiding this comment

aozarov commented May 11, 2015

billy1380 commented May 12, 2015

aozarov commented May 12, 2015

billy1380 commented May 12, 2015

aozarov commented Jul 26, 2015

aozarov commented Jul 27, 2015

billy1380 commented Jul 27, 2015