Skip to content

Add support for metadata to test server#2441

Merged
Quinn-With-Two-Ns merged 3 commits intotemporalio:masterfrom
Quinn-With-Two-Ns:issue-2430
Mar 11, 2025
Merged

Add support for metadata to test server#2441
Quinn-With-Two-Ns merged 3 commits intotemporalio:masterfrom
Quinn-With-Two-Ns:issue-2430

Conversation

@Quinn-With-Two-Ns
Copy link
Contributor

This PR does a few things:

  • Add support for metadata to test server
  • Add summary and detail to describe
  • expose root execution

closes:

@Quinn-With-Two-Ns Quinn-With-Two-Ns requested a review from a team as a code owner March 11, 2025 10:03
* @return Workflow ID of the root Workflow
*/
@Nullable
String getRootWorkflowId();
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 is a bit awkward since this can only be null on old servers that didn't have this value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, may be worth documenting that here in the javadoc. I do appreciate that this is not using Optional.

Copy link
Contributor

@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.

Minor things, nothing blocking

* @return Workflow ID of the root Workflow
*/
@Nullable
String getRootWorkflowId();
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, may be worth documenting that here in the javadoc. I do appreciate that this is not using Optional.

/** Get the fixed summary for this workflow execution. */
@Experimental
@Nullable
public String getStaticSummary() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might consider memoizing this (e.g. backed by an AtomicReference). If not, at least worth clarifying in the javadoc each invocation does data conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We generally don't ever memoize conversion for these type of things, same for memo. I'll note in the docs

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's worth a more generic implementation of command-to-event user metadata propagation like server instead of the only known current use cases. This would make it more future proof. I understand it'd change the code a bit since there is no generic place where an event is created from a command nor is there a common place to affect an event after it is created. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah possibly, child workflow would still need some special logic, but yeah this should be generic

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.

2 participants