From 4f32be6c70f9b8f125196a3beb82e0937658185c Mon Sep 17 00:00:00 2001 From: Chad Retz Date: Thu, 23 Feb 2023 15:52:09 -0600 Subject: [PATCH] Typed search attributes in SDK (#75) --- sdk-typed-search-attributes.md | 350 +++++++++++++++++++++++++++++++++ 1 file changed, 350 insertions(+) create mode 100644 sdk-typed-search-attributes.md diff --git a/sdk-typed-search-attributes.md b/sdk-typed-search-attributes.md new file mode 100644 index 0000000..0c0a8da --- /dev/null +++ b/sdk-typed-search-attributes.md @@ -0,0 +1,350 @@ +# Typed Search Attributes in SDKs + +Today most languages just do arbitrary lists for search attribute values or half-implementations of some-list some not. +This is a problem now with the server having multiple different forms of search attributes and preferring non-lists. +Below is just a proposal of ideas on how better to type search attributes. + +## Current behavior + +### SDK + +* Go + * Search attributes getting in workflow - on info as raw proto `*commonpb.SearchAttributes` + * Search attributes setting in workflow - via `UpsertSearchAttributes` accepting `map[string]interface{}` + * Search attributes setting on start - via `map[string]interface{}` on start options + * Search attributes getting on list/describe - not available in high-level +* Java + * Search attributes getting in workflow - via `getSearchAttributes` for all as `Map>` + * Search attribute getting in workflow - via `getSearchAttribute` for single values and `getSearchAttributeValues` for + multi values (but works for single) + * Search attributes setting in workflow - via `upsertSearchAttributes` accepting `Map` + * Search attributes setting on start - via `Map` on start options + * Search attributes getting on list/describe - as `Map` on list but no high-level describe form + (from latest javadoc, though didn't check repo where we may have new describe form) +* TS + * Search attributes getting in workflow - on info as `Record` (i.e. object) + * Search attributes setting in workflow - via `upsertSearchAttributes` accepting `Record` + * Search attributes setting on start - via `Record` on start options + * Search attributes getting on list/describe - as `Record` +* Python + * Search attributes getting in workflow - on info as `Mapping[str, List[scalar]]` + * Search attributes setting in workflow - via `upsert_search_attributes` accepting `Mapping[str, List[scalar]]` + * Search attributes setting on start - via `Mapping[str, List[scalar]]` on start options + * Search attributes getting on list/describe - as `Mapping[str, List[scalar]]` +* .NET + * Still in alpha, can decide whatever we want + +### Server + +* ElasticSearch + * Multi-value search attribute for non-keyword-list - accepted, but discouraged, and returned as given + * TODO(cretz): Question outstanding to server team about the benefits of multi-value non-keyword-list attributes + * Single-value search attribute for non-keyword-list - accepted and returned as given + * Multi-value search attribute for keyword-list - accepted and returned as given + * Single-value search attribute for keyword-list - fails +* SQL + * Multi-value search attribute for non-keyword-list - only allowed if a single value, always returned single value + * Single-value search attribute for non-keyword-list - accepted and returned as given + * Multi-value search attribute for keyword-list - accepted and returned as given + * Single-value search attribute for keyword-list - fails + +### Problems with current behavior + +* Go and Java rely on user to resolve ambiguity between single and multi value search attributes (Go is worse because + it has no helpers) +* TypeScript and Python always disambiguate making all search attributes a list (and Java too in single-value cases) + even if list not allowed anymore +* Setting invalid search attributes in workflow never returns a failure that can be handled by workflow code +* Users have no way to clarify the difference between keyword and keyword-list + +## Solution + +Problems that we have to overcome: + +* The "search attribute" term is already taken, so we have to have a new name or return a form of the same type +* Upsert has no easy way of determining what is a delete. Today it is using null-ness. + +### Non-solution + +It was decided that it is unreasonable to extend/wrap existing collection types to reuse the "search attribute" name +because: + +* Leaving the existing untyped accessors on the same collection would not encourage anyone to move +* It is hard for us to deprecate untyped accessors in favor of typed ones + +### Details + +* We have decided to create a new strict collection called `SearchAttributes` + * This does not extend/use any existing collection interface in the language + * The collection would have to be a raw tuple because when getting from a server we can't type on a per key basis + * If we reused an existing collection interface, people would be encouraged to use it which has to remove typing + (no language has a good way to represent a collection of values whose types are dependent on their specific key) + * We are gonna make the raw values available via getter instead of on the collection + * We don't want people to have language native collection iteration abilities on this or they'll be working with + untyped tuples + * We considered calling this collection `TypedSearchAttributes` (and lang can if disambiguation needed), but not + wanted + * This collection is read only and languages can provide ways to create it with type-safe key-values the best they can + * Go, Java, Python, and .NET may use a builder, TS may use tuple construction + * The collection should have: + * A single typed getter for getting typed values given a `SearchAttributeKey` + * In Go where methods cannot have generics, this may have to be inverted to add the getter on the attribute + definition + * Ability to determine if key is present and total count in collection + * Ability to retrieve a read-only (ideally sorted or deterministically ranged) collection of raw key/value + string/object tuples + * This needs an unwieldy name like `getRawValues()` + * A builder with a copy constructor +* There is a new generic base type of `SearchAttributeKey` + * Instantiations of this class with certain things can be done via static helpers + * These are just for predefining known search attribute keys and their types + * The `SearchAttributeInt`, `SearchAttributeDouble`, and `SearchAttributeTime` should be named according to language + types. So Go may use `SearchAttributeFloat64`, Java may use `SearchAttributeKey.OffsetDateTime`, etc + * This is expected to be used in top-level consts. So Java might have + `public static final SearchAttributeKey MY_ATTR = new SearchAttributeKey.forInteger("myAttr")` (or can be + just in the workflow interface), Go might have `const MyAttr = SearchAttributeKeyInt("myAttr")`, etc +* We are calling these "typed search attributes" for user getters/setters + * It was considered to just overload client options set and workflow upsert to accept old or new type, but that + doesn't help getters (can't overload the return and union is unclear to users and hard to deprecate) + * It was considered to just move the getter from workflow package/namespace to workflow info object and vice versa, + but this became confusing (in Go they are already on info, in Java they are on workflow context not info, etc) + * It was considered to make this just "search attributes" and find ways per language to cheat that (e.g. in Java + `Workflow.getSearchAttributes` would be deprecated in favor of `Workflow.searchAttributes`) + * This becomes really difficult and inconsistent across languages + * Other names considered were: + * "workflow search attributes" - doesn't work for schedules + * "search attrs" - hard to justify abbreviation + * "attributes" - not acceptable to how Temporal uses the "search attributes" term everywhere +* All existing forms of search attribute getting and setting are hereby deprecated +* Client options will have ability to get/build this collection as "typed search attributes" +* Workflows can get a read-only form of this collection (however lang expresses that best) as "typed search attributes" + getter at the top of the workflow adjacent to where getting "info" is today +* "upsert search attributes" is deprecated + * We could have overloaded this in all languages but Go, but clearer to stay with the "typed search attribute" term +* "upsert typed search attributes" is made available on the workflow level where "upsert" already exists today + * Considered calling "update" or "apply" instead of "upsert", but decided that while "upsert" doesn't represent unset + properly, it matches the event name that users see in the UI + * This accepts a vararg `SearchAttributeUpdate` that is either a "set" or an "unset" + * We accept this is ugly, but there is no better way to represent deletes + * We considered a mutable `SearchAttributes` collection but individual key/value mutations would be too hard to + reason about if buffered/batched and if single upsert commands people would ask for bulk as soon as individual + upsert events appeared + * 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())` + * We intentionally are calling adding "value" prefix instead of just `set` and `unset` to make it clear the key + is providing a value update + * Since we are not offering an async error for this, this function should return the equivalent of + "get typed search attributes" + +## Usage Examples + +### Java + +Public API (this is approximate and untested, just typed here in proposal to get an idea). + +Impl: + +```java +/** Immutable **/ +public class SearchAttributes { + @Nonnull + public T get(SearchAttributeKey key) { + // ... + } + + public boolean containsKey(SearchAttributeKey key) { + // ... + } + + public SortedMap getRawValues() { + // ... + } + + public int size() { + // ... + } + + public static Builder newBuilder() { + // ... + } + + public static Builder newBuilder(SearchAttributes toCopy) { + // ... + } + + public static class Builder { + public Builder set(SearchAttributeKey key, @Nonnull T value) { + // ... + } + + public Builder unset(SearchAttributeKey key) { + // ... + } + + public Builder setUntyped(String key, Object value) { + // ... + } + + public Builder unsetUntyped(String key) { + // ... + } + + public SearchAttributes build() { + // ... + } + } +} + +public abstract class SearchAttributeKey { + SearchAttributeKey(String name, Class valueType) { + // ... + } + + public String getName() { + // ... + } + + public Class getValueType() { + // ... + } + + public SearchAttributeUpdate valueSet(@Nonnull T value) { + // ... + } + + public SearchAttributeUpdate valueUnset() { + // ... + } + + public static SearchAttributeKey> forKeywordList(String name) { + // ... + } + + public static SearchAttributeKey forInteger(String name) { + // ... + } + + // etc + + static class KeywordList extends SearchAttributeKey> { + KeywordList(String name) { + // TODO(cretz): Use Guava type token instead of class to capture generic? + super(name, List.class); + } + } + + static class Integer extends SearchAttributeKey { + Integer(String name) { + super(name, Integer.class); + } + } + + // etc +} + +public abstract class SearchAttributeUpdate { + SearchAttributeUpdate(String key) { } + + public static SearchAttributeUpdate valueSetUntyped(String key, Object value) { + // ... + } + + public static SearchAttributeUpdate valueUnsetUntyped(String key) { + // ... + } + + static class Set extends SearchAttributeUpdate { + // Unfortunately has to be public for untyped use. But + Set(String key, Object value) { + super(key); + // ... + } + } + + static class Unset extends SearchAttributeUpdate { + Unset(String key) { + super(key); + // ... + } + } +} + +// For workflows +public class Workflow { + // ... after other existing stuff including deprecated calls ... + + public SearchAttributes getTypedSearchAttributes() { + // ... + } + + public SearchAttributes upsertTypedSearchAttributes(SearchAttributeUpdate... updates) { + // ... + } +} + +// For clients +public class WorkflowOptions { + // ... after other existing stuff including deprecated calls ... + + public SearchAttributes getTypedSearchAttributes() { + // ... + } + + public static class Builder { + // ... after other existing stuff including deprecated calls ... + + /** Exception thrown if this and search attributes both present */ + public Builder setTypedSearchAttributes(SearchAttributes attributes) { + // ... + } + } +} +``` + +Usage: + +```java +public interface MyWorkflow { + SearchAttributeKey MY_ATTR = new SearchAttributeKey.forInteger("my-attr-name"); + + // ... +} + +// Client usage +public class Main { + public static void main(String[] args) { + // ... + + var workflow = client.newWorkflowStub( + MyWorkflow.class, + WorkflowOptions.newBuilder() + .setWorkflowId("my-id") + .setTaskQueue(taskQueue) + .setTypedSearchAttributes(SearchAttributes + .newBuilder() + .set(MyWorkflow.MY_ATTR, 123) + .build()) + .build() + ); + + // ... + } +} + +// Inside workflow usage +public class MyWorkflowImpl implements MyWorkflow { + + @Override + public void run() { + // ... + + logger.info("Orig search attr: {}", Workflow.getTypedSearchAttributes().get(MY_ATTR)); + + Workflow.upsertTypedSearchAttributes(MY_ATTR.valueSet(456)); + + logger.info("New search attr: {}", Workflow.getTypedSearchAttributes().get(MY_ATTR)); + + // ... + } +} +``` \ No newline at end of file