EarlyReturn sample (Update-With-Start) #689

steveandroulakis · 2024-10-18T19:24:22Z

If it is a significant code change, please make sure there is an open issue for this.
We work best with you when we have accepted the idea first before you code. -->

What was changed

Sample that uses the new Update-With-Start feature.

Why?

Help users understand what Update-With-Start can do with a simple financial transaction example.

Based on the Go sample here: https://github.com/temporalio/samples-go/tree/main/early-return

Closes
No issue
How was this tested:
I welcome guidance on how to effectively write tests for this!
Any docs updates needed?
No

CLAassistant · 2024-10-18T19:24:29Z

All committers have signed the CLA.

mfateev · 2024-10-18T21:58:54Z

We should either use the proper Java style of separating classes and interfaces into their own files or implement this as another Hello sample.

core/src/main/java/io/temporal/samples/earlyreturn/EarlyReturn.java

steveandroulakis · 2024-10-20T19:40:05Z

We should either use the proper Java style of separating classes and interfaces into their own files or implement this as another Hello sample.

Thanks, will split into separate files.

drewhoskins-temporal

optional nit

drewhoskins-temporal · 2024-10-23T16:20:22Z

core/src/main/java/io/temporal/samples/earlyreturn/TransactionWorkflowImpl.java

+  }
+
+  @Override
+  public String returnInitResult() {


nit: We generally try to set a good example and return structures rather than scalars for future compat of being able to add new fields. I was asked to improve a similar situation when I wrote a python sample, so figured I'd echo the advice here. FF to ignore since I'm not sure how consistent we really are and understand it's orthogonal.

Quinn-With-Two-Ns · 2024-10-24T20:15:01Z

core/src/main/java/io/temporal/samples/earlyreturn/TransactionWorkflowImpl.java

+
+  @Override
+  public TxResult processTransaction(Transaction txInput) {
+    this.tx = txInput;


I would refactor this sample to make Transaction immutable and not modify transaction as the workflow progress. I would make two different classes one for the workflow input that doesn't have a transaction ID field and one (maybe returned by the activity initTransaction or created in the workflow) that the cancelTransaction and completeTransaction

Done: creation of a TransactionRequest object + the Transaction class is now immutable

core/src/main/java/io/temporal/samples/earlyreturn/TransactionWorkflowImpl.java

Quinn-With-Two-Ns · 2024-10-24T20:17:45Z

@stephanos May want to take a look

Quinn-With-Two-Ns · 2024-10-24T20:20:46Z

A unit test showing update with start would be great, @stephanos but a lot of work into the time skipping test server part, but I wouldn't block the PR on it if you don't have time.

steveandroulakis · 2024-10-25T15:50:26Z

A unit test showing update with start would be great, @stephanos but a lot of work into the time skipping test server part, but I wouldn't block the PR on it if you don't have time.

I have written 2 unit tests. I hope they're done correctly!

Quinn-With-Two-Ns · 2024-10-25T15:54:14Z

core/src/main/java/io/temporal/samples/earlyreturn/TransactionActivitiesImpl.java

+  }
+
+  @Override
+  public void cancelTransaction(TransactionRequest txRequest) {


I think cancelTransaction should also take the Transaction class since, in practice, you would need the transaction ID to cancel it right?

I'm diverging more and more from the original Go sample (which is fine).

Right now, I think of an initialization as something that results in the 'minting' of a Transaction ID. And a failure of initialization causes an activity failure which triggers a cancellation.

I can't think of a way of having

InitTransaction cause an activity failure (fundamental to @stephanos original sample) AND

Returning a transaction ID that would mean the creation of a Transaction object I can then use in cancelTransaction

Thinking blank-slate for a moment, and about the purpose of an "Early Return" type sample. @Quinn-With-Two-Ns -- What do you think you would want to see from a scenario where a transaction fails on initialization (e.g. the workflow update with start)? What data should be returned and what compensation should take place after? I don't think I should spend any more hours chewing on this sample without figuring that out first. Thanks!

Yeah you raise a great point. I think I would separate the creation of the ID from the initialization of the transaction. Treating the transaction ID as an idempotency key for the rest of the activities. That being said I think this sample is really good as is and an improvement over the Go Sample so
wouldn't hold up the PR on this

core/src/main/java/io/temporal/samples/earlyreturn/TransactionActivities.java

core/src/main/java/io/temporal/samples/earlyreturn/TransactionActivitiesImpl.java

…Activities.java Co-authored-by: Stephan Behnke <[email protected]>

stephanos · 2024-10-25T16:34:58Z

core/src/main/java/io/temporal/samples/earlyreturn/EarlyReturnClient.java

+  private static WorkflowOptions buildWorkflowOptions() {
+    return WorkflowOptions.newBuilder()
+        .setTaskQueue(TASK_QUEUE)
+        .setWorkflowId(WORKFLOW_ID_PREFIX + System.currentTimeMillis())


In the Go example, I'm generated the transaction ID on the client and use it in the Workflow ID. That way, the user would receive an error when they start a workflow for the same transaction again. I don't think it's strictly necessary to mimic that here, but I thought I'd mention it.

Gotcha. I intentionally changed it so the early-return would contain meaningful data usable for the rest of the transaction (an early-return / initialization step that returns null isn't very illustrative of real-world value imo)

I plan on following Quinn's suggestion here of still having the ID minted as part of an activity, but doing it in a separate one as part of the initialization step #689 (comment) -- Let me know if there's anything you'd do differently

stephanos · 2024-10-25T16:41:02Z

core/src/test/java/io/temporal/samples/earlyreturn/TransactionWorkflowTest.java

+    assertTrue(appFailure.getMessage().contains("Invalid Amount"));
+
+    // Let workflow process error handling
+    testWorkflowRule.getTestEnvironment().sleep(Duration.ofSeconds(1));


Out of curiosity, is this sleep necessary? I would have expected the getResult further down would just block until the result is available.

You're absolutely right. There's no need as getResult blocks for a response. Removing

Add API key option to Nexus sample

EarlyReturn sample (Update-With-Start)

284f4fa

steveandroulakis requested review from tsurdilo, antmendoza and a team as code owners October 18, 2024 19:24

Quinn-With-Two-Ns reviewed Oct 20, 2024

View reviewed changes

core/src/main/java/io/temporal/samples/earlyreturn/EarlyReturn.java Outdated Show resolved Hide resolved

Quinn-With-Two-Ns reviewed Oct 20, 2024

View reviewed changes

core/src/main/java/io/temporal/samples/earlyreturn/EarlyReturn.java Outdated Show resolved Hide resolved

steveandroulakis added 4 commits October 21, 2024 16:22

multifile but typed stub doesn't work

5082bb6

multifile but typed stub doesn't work

d38125b

split files, using jackson for transaction object

86c4883

spotlessApply

497f2dc

drewhoskins-temporal reviewed Oct 23, 2024

View reviewed changes

steveandroulakis added 2 commits October 23, 2024 09:40

workflow and update returns objects now

d161cbd

workflow and update returns objects

7700fa1

Quinn-With-Two-Ns reviewed Oct 24, 2024

View reviewed changes

core/src/main/java/io/temporal/samples/earlyreturn/TransactionWorkflowImpl.java Show resolved Hide resolved

steveandroulakis added 2 commits October 25, 2024 07:46

immutable transaction class

463c2c7

Early Return UpdateWithStart unit test

78bd0c2

Quinn-With-Two-Ns reviewed Oct 25, 2024

View reviewed changes

Quinn-With-Two-Ns approved these changes Oct 25, 2024

View reviewed changes

stephanos reviewed Oct 25, 2024

View reviewed changes

core/src/main/java/io/temporal/samples/earlyreturn/TransactionActivities.java Outdated Show resolved Hide resolved

stephanos reviewed Oct 25, 2024

View reviewed changes

core/src/main/java/io/temporal/samples/earlyreturn/TransactionActivitiesImpl.java Show resolved Hide resolved

Update core/src/main/java/io/temporal/samples/earlyreturn/Transaction…

b8513d9

…Activities.java Co-authored-by: Stephan Behnke <[email protected]>

stephanos reviewed Oct 25, 2024

View reviewed changes

steveandroulakis and others added 5 commits October 25, 2024 10:22

remove sleep from test

9037387

Minting has its own activity

cb93bfb

Fix nexus sample TLS settings (temporalio#688)

f40e832

Add API key option to Nexus sample (temporalio#690)

ce6cc56

Add API key option to Nexus sample

Bump Java SDK to v1.26.1 (temporalio#695)

8095cc7

Quinn-With-Two-Ns merged commit 58124a8 into temporalio:main Oct 29, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EarlyReturn sample (Update-With-Start) #689

EarlyReturn sample (Update-With-Start) #689

steveandroulakis commented Oct 18, 2024

CLAassistant commented Oct 18, 2024 •

edited

Loading

mfateev commented Oct 18, 2024

steveandroulakis commented Oct 20, 2024

drewhoskins-temporal left a comment

drewhoskins-temporal Oct 23, 2024

steveandroulakis Oct 25, 2024

Quinn-With-Two-Ns Oct 24, 2024

steveandroulakis Oct 25, 2024

Quinn-With-Two-Ns commented Oct 24, 2024

Quinn-With-Two-Ns commented Oct 24, 2024

steveandroulakis commented Oct 25, 2024

Quinn-With-Two-Ns Oct 25, 2024

steveandroulakis Oct 25, 2024 •

edited

Loading

Quinn-With-Two-Ns Oct 25, 2024

stephanos Oct 25, 2024

steveandroulakis Oct 25, 2024

stephanos Oct 25, 2024 •

edited

Loading

steveandroulakis Oct 25, 2024

EarlyReturn sample (Update-With-Start) #689

EarlyReturn sample (Update-With-Start) #689

Conversation

steveandroulakis commented Oct 18, 2024

What was changed

Why?

CLAassistant commented Oct 18, 2024 • edited Loading

mfateev commented Oct 18, 2024

steveandroulakis commented Oct 20, 2024

drewhoskins-temporal 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

Quinn-With-Two-Ns commented Oct 24, 2024

Quinn-With-Two-Ns commented Oct 24, 2024

steveandroulakis commented Oct 25, 2024

Choose a reason for hiding this comment

steveandroulakis Oct 25, 2024 • edited Loading

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

stephanos Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Oct 18, 2024 •

edited

Loading

steveandroulakis Oct 25, 2024 •

edited

Loading

stephanos Oct 25, 2024 •

edited

Loading