-
Notifications
You must be signed in to change notification settings - Fork 18
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
Propose new search attribute APIs #67
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,86 @@ | ||||||||
## Search attributes V2 | ||||||||
|
||||||||
### Why do we need this? | ||||||||
|
||||||||
In the current version of the SDK, search attributes are always arrays. In order for the server to support visibility | ||||||||
on top of standard persistence, we need to redo the search attributes API to enforce single values for all search | ||||||||
attributes except for keywords. The server will eventually drop support for multiple search attribute values for | ||||||||
non-keywords. | ||||||||
|
||||||||
### Minimal proposal | ||||||||
|
||||||||
Make the E2E experience consistent, including query construction for listing workflows and search attribute | ||||||||
registration. | ||||||||
|
||||||||
`workflows.ts` | ||||||||
|
||||||||
```ts | ||||||||
import { searchAttributes } from '@temporalio/common'; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just a set of functions and constant, it includes things like:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can I iterate all of them inside the workflow? Can I do things like unset all of the ones with a certain prefix? (i.e. a replace-all) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'd have to define your own set of search attributes, it's up to you how to access those. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeScript doesn't allow object destructuring on imports statements. That means that one would always have to write I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like 2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like 2 as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "built in search attributes" should be grouped together. Not formally an enum, but an object on which every field is a builtin search attribute. Having an export named simply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, maybe have a |
||||||||
|
||||||||
export const customerAttribute = searchAttributes.defineAttribute(searchAttributes.Text, 'customer'); | ||||||||
export const dateOfPurchaseAttribute = searchAttributes.defineAttribute(Date, 'dateOfPurchase'); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might drop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be fine with just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kind of like the verb |
||||||||
|
||||||||
export async function myWorkflow() { | ||||||||
// Get the value of a search attribute - returns string | undefined here | ||||||||
const customer = customerAttribute.value(); | ||||||||
// ... random workflow logic ... | ||||||||
|
||||||||
// Set the value of a search attribute, all set and unset calls are buffered and flushed out as a single command at | ||||||||
// the end of an activation | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I assume last write wins when altering the same attribute before next command? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I thought that was obvious but I can add a comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was mostly obvious, no comment needed |
||||||||
dateOfPurchaseAttribute.set(new Date()); | ||||||||
// Explicit unset API | ||||||||
dateOfPurchaseAttribute.unset(); | ||||||||
// ... | ||||||||
} | ||||||||
``` | ||||||||
|
||||||||
`client.ts` | ||||||||
|
||||||||
```ts | ||||||||
import { searchAttributes } from '@temporalio/common'; | ||||||||
import { Client, Q } from '@temporalio/client'; | ||||||||
import { customerAttribute, dateOfPurchaseAttribute, myWorkflow } from './workflows'; | ||||||||
|
||||||||
const client = new Client(/* options */); | ||||||||
|
||||||||
// Only available in OSS for now | ||||||||
await client.operator.registerSearchAttributes(customerAttribute, dateOfPurchaseAttribute); | ||||||||
// Other methods will be exposed too | ||||||||
|
||||||||
const iterator = client.workflow.list({ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quick question as I start to work on this in Python. So this solves temporalio/features#75 too? I assume this is interceptable? Also, can you show a quick example of status filtering? Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will solve the high level SDK list API, assuming you want to add the query builder (you should IMHO). For search attributes that we know are enums I'd definitely restrict the type (I think it's only ExecutionStatus). Something like: Q.filter(builtinSearchAttributes.ExecutionStatus.eq('RUNNING')) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
While I thought similar, I was overridden on workflow handle "describe". I think we are now forced to be consistent here. |
||||||||
query: Q.filter( | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. object |
||||||||
// WorkflowType is a pre-defined search attribute | ||||||||
Q.and(searchAttributes.WorkflowType.eq(myWorkflow.name), customerAttribute.eq('customer-id-1234')).orderBy( | ||||||||
mjameswh marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
dateOfPurchaseAttribute, | ||||||||
'ASC' // Optionally specify DESC | ASC | ||||||||
) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks nice but I wouldn't underestimate the complexity of a query builder. This isn't a small amount of work to implement properly, especially if it's type-safe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I already built most of it while designing this and it's not my first time building one of these. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me it's not about difficulty of building, it's about maintenance and compatibility. How well defined are list filters? If you can give me a grammar that you guarantee won't change in an incompatible way, then I can support a programmatic builder. Even then though, though there's some type safety, you'll end up where SQL query builders always end up where people will escape back into string literals for extreme flexibility. Also, this API needs to be fledged out some more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked with the server team and they've confirmed that the entire list of capabilities is documented here: https://docs.temporal.io/concepts/what-is-a-list-filter. I'm not proposing this as the only way, The The doc page mentions list filters but it actually talks about queries, while the projection part of the query is omitted in Temporal, the rest is a query, it's composed of filters and ordering operators. Essentially Q is a builder. You can compose multiple operations:
NOTE that queries are immutable and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this builder solving a need people have? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoiding typos There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, to clarify I mean as in a struggle people have today. They can typo lots of strings, e.g. task queues, I am just wondering if anyone is clamoring for this. Also, can we get an exact query grammar? (if not in this issue, just in docs somewhere) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The doc page I linked above. I don't think people are clamoring for this, I just this it's a general improvement and is pretty straightforward to do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe not "clamoring", but I would have appreciated a query builder for the one spot in the server where I had to construct a query for internal use. Mostly just so I don't have to think about quoting or escaping. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'll clamor to avoid thinking about quotes in quotes 😊 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can constructing a filter always succeed? or should we have a users call a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They always succeed or throw on construction. |
||||||||
), | ||||||||
pageSize: 100, // optional | ||||||||
}); | ||||||||
|
||||||||
// Returns something like the TS wrapper returned by Describe - the API returns | ||||||||
// temporal.api.workflow.v1.WorkflowExecutionInfo | ||||||||
for await (const { workflowId, ...rest } of iterator) { | ||||||||
const handle = client.workflow.getHandle(workflowId); | ||||||||
await handle.cancel(); | ||||||||
} | ||||||||
|
||||||||
const handle = client.getHandle(someWorkflowId); | ||||||||
const info = await handle.describe(); | ||||||||
const customer = info.getAttribute(customerAttribute); // includes runtime validation - returns string | undefined | ||||||||
// -- OR -- | ||||||||
const customer = info.getAttribute<string>(customer); // no runtime validation - returns string | undefined | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this argument supposed to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, thanks for catching it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works for me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should standardize on the word "search attribute" or "attribute". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we should probably just stick to search attribute. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you'll do the same for memo here and for the in-workflow part? (though memo can be any type and is not part of your filter work) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no need to do this for memo, but I do think that the "upsert memo" API in the workflow module will do batching as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Getting a single memo value by key for a specific type makes sense though, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm.. right now memos can be any user object so we can't do runtime validation like we do for search attributes. Adding the getter doesn't provide that much value IMHO but I'll consider adding it for API consistency with search attributes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We added workflow.memo and workflow.memo_value (and the client handle describe response equivalents) after discussion because there is no simple way to deserialize a memo value without manually invoking a converter. No need to add it now, but I think making memo and search attributes APIs match as much as possible makes sense (though obviously the latter is much more limited on types). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we had type hints in TS I'd do it, but we currently don't support that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we mark this new API experimental until implemented in another SDK? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we should mark any new APIs experimental for a certain period IMHO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
``` | ||||||||
|
||||||||
### What about the current APIs? | ||||||||
|
||||||||
Search attributes are exposed in the SDK today in 3 places: | ||||||||
|
||||||||
1. In the client - `WorkflowClient.describe()` return value (`WorkflowExecutionDescription.searchAttributes`) | ||||||||
2. In the workflow - `workflowInfo()` return value (`WorkflowInfo.searchAttributes`) | ||||||||
3. `upsertSearchAttributes` call from the workflow | ||||||||
|
||||||||
We'll deprecate all of these APIs and point to the alternatives presented above. | ||||||||
|
||||||||
If `upsertSearchAttributes` is called in the same activation after as the new `set|unset` APIs are used, it will trigger | ||||||||
flushing the buffered changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could maintain the array & support for multiple values when backed by ES, while supporting single-value arrays for standard persistence. There are use cases for it. Was it decided that there are not enough use cases to warrant the complexity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want different experience based on the backend.
Research shows that multiple values are almost never used.