Skip to content
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.

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

Add support for user meta data #2218

Merged
merged 8 commits into from
Sep 16, 2024

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

Add support for user metadata on workflows and timer events. Other events can be added in the future, just picked these events to match the Go SDK.

Note: This does NOT cover __temporal_workflow_metadata, that feature is tracked here

closes #2216

@Quinn-With-Two-Ns Quinn-With-Two-Ns requested a review from a team as a code owner September 15, 2024 06:42
* <p>Default is none/empty.
*/
@Experimental
public Builder setStaticSummary(String staticSummary) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not gonna work until temporalio/temporal#6446 is in a released server version, unsure if you want to wait to expose until then. I do see the test is skipped currently for real servers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this should be in 1.25.0 and the tests are skipped for the test server, not the real server

Copy link
Member

Choose a reason for hiding this comment

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

Hrmm, child workflows may not have made the initial version (they weren't initially wired up). @gow - can you confirm whether or not the child workflow user metadata propagation is in a tagged server version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Yes. It's included in 1.25 release - temporalio/temporal@v1.25.0...main

import org.junit.Rule;
import org.junit.Test;

public class ChildWorkflowMetadataTest {
Copy link
Member

Choose a reason for hiding this comment

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

Any integration/e2e tests for regular (i.e. non-child) workflows with static summary/details? Or is that more difficult because it requires test server support?

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 test also covers that, we test the parent workflow as well

@@ -89,6 +89,11 @@ public static Promise<Void> newTimer(Duration duration) {
return getWorkflowOutboundInterceptor().newTimer(duration);
}

public static Promise<Void> newTimer(Duration duration, TimerOptions options) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cretz one thing I debated on was if I should include duration in TimerOptions. We didn't in Go, but I didn't couldn't find a strong reason why?

Copy link
Member

@cretz cretz Sep 16, 2024

Choose a reason for hiding this comment

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

No strong reason. We just figured in Go (and other langs?) that having required duration in both overloaded forms makes sense, and these are just "additional" options beyond duration. If we want to change that stance (here or there), I have no strong opinion, and now would be the time to do it.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

One minor thing, then LGTM

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit 03f7182 into temporalio:master Sep 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MVP support for user meta data on workflows and events
3 participants