Skip to content

Add DescribeWorkflowExecution to TestWorkflowService #670

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

Jump to bottom
Merged
merged 5 commits into from
Aug 30, 2021
Merged

Add DescribeWorkflowExecution to TestWorkflowService #670

merged 5 commits into from
Aug 30, 2021

Conversation

nagl-stripe
Copy link
Contributor

@nagl-stripe nagl-stripe commented Aug 25, 2021 •
edited
Loading

What was changed

This change adds support for DescribeWorkflowExecution to the test service. It should support everything the real server
supports except for AutoResetPoints, which the test service doesn't otherwise support (for good reason).

Implementation-wise, this is just a port of HistoryEngine.DescribeWorkflowExecution from golang, except that we infer some things from history that the real service has in its database.

Why?

In Stripe, we use the java test service to help people test business logic that starts/interacts-with workflows at build time without using a real temporal server. Several times, we've wanted to call describe in these code paths (e.g. to get diagnostic information about a workflow) but haven't, because it isn't supported in the test service.

Checklist

  1. Closes DescribeWorkflowExecution is not implemented for the java test service. #514

  2. How was this tested:
    There's a new unit test that exercises Describe against workflows in various states.

export USE_DOCKER_SERVICE=true
gradlew test # there are failures here, but they happen with or without this commit
export USE_DOCKER_SERVICE=false
gradlew test # no failures before or after this comimt
  1. Any docs updates needed?
    No

@nagl-stripe
Add DescribeWorkflowExecution to TestWorkflowService
de2f076 
This change adds support for DescribeWorkflowExecution to the
test service. It should support everything the real server
supports except for AutoResetPoints, which the test service
doesn't otherwise support (for good reason).

Implementatoin-wise, this is just a port of
HistoryEngine.DescribeWorkflowExecution from golang, except that
we infer some things from history that the real service just
stores.
nagl-stripe
nagl-stripe commented Aug 25, 2021
View reviewed changes
temporal-testing/src/main/java/io/temporal/internal/testservice/TestWorkflowStoreImpl.java
if (!getRequest.getWaitNewEvent()
&& getRequest.getHistoryEventFilterType()
!= HistoryEventFilterType.HISTORY_EVENT_FILTER_TYPE_CLOSE_EVENT) {
if (!getRequest.getWaitNewEvent()) {
Copy link
Contributor Author

@nagl-stripe nagl-stripe Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

A previous iteration of the describe code called getWorkflowExecutionHistory(FILTER_TYPE_CLOSE_EVENT, waitNewEvent=false). Because of the logic in red, it would wait even though it was told not to.

This fix isn't needed for the current iteration of the code, and I can use a separate PR for it if you want. Or we can just not fix it.

@nagl-stripe
Copy link
Contributor Author

What's the right way to see the build failure? I have a buildkite account, but I think I need an invite to your organization to see your builds.

@nagl-stripe
Fix some lint
ed6646c
@nagl-stripe
Remove assertions on history length - in a constrained execution envi…
75ffade 
…ronment (like at build-time), we can't always know how many history entries to expect.
Spikhalskiy
Spikhalskiy approved these changes Aug 29, 2021
View reviewed changes
Copy link
Contributor

@Spikhalskiy Spikhalskiy left a comment

Choose a reason for hiding this comment

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

Thank you for the amazing contribution.
I left some comments regarding readability, code reuse and duplicate, and general code style.
Otherwise LGTM.
Ping me when the comments are addressed and I will help with merging.

temporal-testing/src/main/java/io/temporal/internal/testservice/StateMachines.java Outdated
this.terminal = terminal;
}

public boolean isTerminal() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok to put this flag here, but could you please refactor out TestWorkflowMutableStateImpl#isTerminalState then, otherwise we have it in two different places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, didn't even see that. I'll switch to using it and remove this.

temporal-testing/src/main/java/io/temporal/internal/testservice/StateMachines.java Outdated
}
}

public static boolean historyEventIsTerminal(HistoryEvent e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is WorkflowExecutionUtils#isWorkflowExecutionCompletedEvent for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will switch.

temporal-sdk/src/test/java/io/temporal/workflow/shared/DescribeWorkflowAsserter.java
/*
* Fluent assertions (ala truth or assert-j) for DescribeWorkflowResults.
*/
public class DescribeWorkflowAsserter {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 like this style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The two libraries mentioned in the comment give you this style for primitives/collections/exceptions. Consider adopting them, particularly if you're tired of people transposing the args to junit's assert.

Truth in particular has some proto-related features, but I find them only mildly useful.

...ral-testing/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java Outdated
return builder.build();
}

private Timestamp timestampFromMillis(long millis) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use com.google.protobuf.util.Timestamps#fromMillis instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this - I thought I had switched once I discovered it, but clearly not.

...ral-testing/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java
WorkflowExecutionInfo.Builder executionInfo = WorkflowExecutionInfo.newBuilder();
executionInfo
.setExecution(this.executionId.getExecution())
.setType(this.getStartRequest().getWorkflowType())
Copy link
Contributor

Choose a reason for hiding this comment

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

There is some mix of styles here. Accessing private fields through this, without this, using getters. It looks like this class is written mostly using the second style, so let's normalize code style in these new methods.

...ral-testing/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java Outdated
}
}

if (scheduledEvent.hasRetryPolicy()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

scheduledEvent. retryPolicy and pendingActivity.retryState.retryPolicy are the same thing. If you replace the first one with the second one here - the code will become cleaner and easier to read.
Also after that, this method can be nicely split into two more granular ones: one is publishing from scheduledEvent only, another is publishing from pendingActivity only.

...ral-testing/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java Outdated
// We don't track this in the test environment right now, but we could.
builder.setLastWorkerIdentity("test-environment-worker-identity");
} else {
builder.setAttempt(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you replace builder.setAttempt(pendingActivity.retryState.getAttempt()); with builder.setAttempt(pendingActivity.getAttempt()); and move it from the if statement - you can kill this else completely, because retryState != null ? retryState.getAttempt() : 1 logic is already located inside pendingActivity.getAttempt()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can clean this up even further - in StateMachines.scheduleActivityTask, .retryState is always set and the attempt number starts at 1 and gets mutated from there.

I'm 99% sure retryState is never null and retryPolicy is never null (but may have been in very old schemas, because the golang code makes allowances for those cases). Let's confirm/deny over slack.

...ral-testing/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java Outdated
}
}

private void setTimestampsBasedOnHistory(
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a setter. Please consider renaming to something like fillExecutionInfoTimestampsBasedOnHistory and making this method, getStartEvent and getCompletionEvent static.

@nagl-stripe
Copy link
Contributor Author

So much cleaner now - thank you for the feedback. I think this is ready to go.

@Spikhalskiy Spikhalskiy merged commit 206d758 into temporalio:master Aug 30, 2021
@nagl-stripe nagl-stripe deleted the describe branch August 30, 2021 19:58
jeffschoner-stripe pushed a commit to jeffschoner-stripe/sdk-java that referenced this pull request Sep 29, 2021
@nagl-stripe
Add DescribeWorkflowExecution to TestWorkflowService (temporalio#670)
ec97cdd 
This change adds support for DescribeWorkflowExecution to the
test service. It should support everything the real server
supports except for AutoResetPoints, which the test service
doesn't otherwise support (for good reason).

Implementatoin-wise, this is just a port of
HistoryEngine.DescribeWorkflowExecution from golang, except that
we infer some things from history that the real service just
stores.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mfateev mfateev Awaiting requested review from mfateev

@Sushisource Sushisource Awaiting requested review from Sushisource

@vitarb vitarb Awaiting requested review from vitarb

1 more reviewer

@Spikhalskiy Spikhalskiy Spikhalskiy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DescribeWorkflowExecution is not implemented for the java test service.

2 participants

@nagl-stripe @Spikhalskiy