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

Typed search attributes in SDK #75

Merged
merged 2 commits into from
Feb 23, 2023

Conversation

cretz
Copy link
Member

@cretz cretz commented Feb 17, 2023

Rendered (until merged): https://github.com/cretz/temporal-proposals/blob/sdk-typed-search-attributes2/sdk-typed-search-attributes.md

This is a followup after discussion from #74 and supersedes that one.

@cretz cretz requested review from rodrigozhou, alexshtin and a team February 17, 2023 20:19
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

Definitely prefer this to first proposal 👍

* Each `SearchAttributeKey` has a `valueSet(value)` and a `valueUnset()` that return `SearchAttributeUpdate`s
* So Java may look like `Workflow.updateSearchAttributes(MY_ATTR1.valueSet("foo"), MY_ATTR2.valueUnset())`
* Ok that we call it "unset" instead of "delete"?
* Ok that we call it `valueSet` and `valueDelete` instead of just `set` and `delete`?
Copy link
Member

Choose a reason for hiding this comment

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

I think these are fine but I'd just switch it around to be setValue and unsetValue

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I put the noun first is because the verb makes you think you're doing something instead of just building an update

@cretz
Copy link
Member Author

cretz commented Feb 23, 2023

I have made updates based on discussion. Namely, SearchAttributes collection is immutable and a builder pattern is required to build it for client, child, and continue-as-new need.

@cretz cretz force-pushed the sdk-typed-search-attributes2 branch from 9d2c07f to 473771c Compare February 23, 2023 14:52
@cretz cretz force-pushed the sdk-typed-search-attributes2 branch from 473771c to 7a1fb6f Compare February 23, 2023 15:56
@cretz
Copy link
Member Author

cretz commented Feb 23, 2023

Merging now that discussion is through. However, we're already learning things on the implementation side that are changing some of this (i.e. that we know the type of every key from the server payload).

@cretz cretz merged commit 4f32be6 into temporalio:master Feb 23, 2023
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.

3 participants