8000 Enhancements to the Early Return sample by steveandroulakis · Pull Request #702 · temporalio/samples-java · GitHub
[go: up one dir, main page]
Skip to content

Enhancements to the Early Return sample #702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Enhancements to the Early Return sample #702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
02ff9ef
client error detailed logged (to catch ExecuteMultiOperation disabled…
steveandroulakis Nov 10, 2024
138a41d
code comments
steveandroulakis Nov 10, 2024
c693a65
Early Return in main README
steveandroulakis Nov 10, 2024
ef45d74
Explicit catching of ExecuteMultiOperation exception cause
steveandroulakis Nov 12, 2024
e05af76
spotless fixes
steveandroulakis Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,9 @@ See the README.md file in each main sample directory for cut/paste Gradle comman

- [**Exclude Workflow/ActivityTypes from Interceptors**](/core/src/main/java/io/temporal/samples/excludefrominterceptor): Demonstrates how to exclude certain workflow / activity types from interceptors.

- [**Early Return**](/core/src/main/java/io/temporal/samples/earlyreturn): Demonstrates how a client can start a new workflow and synchronously receive
a response mid-workflow, while the workflow continues to run to completion (using Update-with-Start).

#### SDK Metrics

- [**Set up SDK metrics**](/core/src/main/java/io/temporal/samples/metrics): Demonstrates how to set up and scrape SDK metrics.
Expand Down
20 changes: 19 additions & 1 deletion core/src/main/java/io/temporal/samples/earlyreturn/EarlyReturnClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,25 @@ private static void runWorkflowWithUpdateWithStart(WorkflowClient client) {
+ result.getTransactionId()
+ ")");
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in description you mention specific grpc exception (StatusRuntimeException)
here just general Exception is asumed its of that type. could add some type check and maybe show how different exceptions could be handled differently in client

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsurdilo I have provided a check for StatusRuntimeException and "ExecuteMultiOperation API is disabled" specifically (with advice). Please let me know if this is enough.

System.err.println("Transaction initialization failed: " + e.getMessage());
if (e.getCause() instanceof io.grpc.StatusRuntimeException) {
io.grpc.StatusRuntimeException sre = (io.grpc.StatusRuntimeException) e.getCause();

System.err.println("Workflow failed with StatusRuntimeException: " + sre.getMessage());
System.err.println("Cause: " + e.getCause());

if (sre.getStatus().getCode() == io.grpc.Status.Code.PERMISSION_DENIED
&& sre.getMessage()
.contains("ExecuteMultiOperation API is disabled on this namespace")) {

// Inform the user that UpdateWithStart requires the ExecuteMultiOperation API to be
// enabled
System.err.println(
"UpdateWithStart requires the ExecuteMultiOperation API to be enabled on this namespace.");
}
} else {
System.err.println("Transaction initialization failed: " + e.getMessage());
System.err.println("Cause: " + e.getCause());
}
}
}

Expand Down
7 changes: 4 additions & 3 deletions core/src/main/java/io/temporal/samples/earlyreturn/TransactionWorkflowImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public TxResult processTransaction(TransactionRequest txRequest) {
} catch (Exception e) {
initError = e;
} finally {
initDone = true;
initDone = true; // Will unblock the early-return returnInitResult method
}

if (initError != null) {
Expand All @@ -60,13 +60,14 @@ public TxResult processTransaction(TransactionRequest txRequest) {

@Override
public TxResult returnInitResult() {
Copy link
Contributor
@dandavison dandavison Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the early return update should return a distinct result type from the workflow. In my samples I've been imagining that the early return type is a "confirmation" or "validation" of the transaction, whereas the final workflow result would be some sort of statement of the final outcome of the transaction (I'm not an expert in this domain, but perhaps the exact amount transferred is not known until a later stage, e.g. due to market fluctuations/exchange rates etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is for some logging enhancements to an already-accepted sample. It sounds like we're going back to the drawing board somewhat on what the sample should do?

For clarity, currently the outcome of the early-return is for example:

      {
        "transactionId": "TXID8524360451",
        "status": "Initialization successful"
      }

(the update mints a transaction id such as TXID8524360451)

And the workflow result is:

{
  "transactionId": "TXID8524360451",
  "status": "Transaction completed successfully."
}

@dandavison Please tell me how do better express the 'validation' vs 'final confirmation' step and I will look at making the changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave them until a subsequent PR; we may still be making some adjustments to the API. I think one underlying issue is that the Go sample doesn't have a return type for the update. So yes, I think it would be clearest for users if the update and the workflow return distinct types, e.g. the update should return TransactionConfirmation(transactionId, confirmationCode, ...) and the workflow should return TransactionStatement(transactionId, finalAmount, ...). But yes, don't worry, I can make the changes, and I'll talk to @drewhoskins-temporal to de-naiveify the financial concepts.

Copy link
Contributor Author
@steveandroulakis steveandroulakis 8000 Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit, made this reply before seeing the one you sent above, Dan.

It sounds like if I introduce a new object e.g. TxInitiationResult and make it return some stuff that feels very "this has been successfully initialized!" and keep the TxResult object for the workflow result that would satisfy.

I want to confirm this though, as I'm re-hashing a sample that was already accepted as-is and I'm trying to avoid endless churn on this. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good. I'll get confirmation and post back here in an hour or less.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, @drewhoskins-temporal and I agree that distinct return types are desirable. But like I say, if you want to leave it then I can do it as I suspect there's going to be a tweak to the API coming soon.

Workflow.await(() -> initDone);
Workflow.await(() -> initDone); // Wait for the initialization step of the workflow to complete
5B3A
if (initError != null) {
log.info("Initialization failed.");
throw Workflow.wrap(initError);
}

return new TxResult(tx.getId(), "Initialization successful");
return new TxResult(
tx.getId(), "Initialization successful"); // Return the update result to the caller
}
}
Loading
0