Skip to content

Conversation

vitarb
Copy link
Contributor

@vitarb vitarb commented May 11, 2021

What was changed:

Custom data converters used with search attributes and memo result in failures down the line when we try to put data into ES.

Why?

Proposal that may address #468

@GreyTeardrop
Copy link
Contributor

I wonder if there can still be a reason to use a custom data converter for search attributes? E.g. if Kotlin data class is used as attribute value then KotlinModule should be registered with the ObjectMapper used by the converter.

@Spikhalskiy
Copy link
Contributor

Spikhalskiy commented May 11, 2021

Just to echo @GreyTeardrop comment. If search attributes can be any object (judging from the fact it's Map<String, Object>) - we need to have custom converters. Otherwise, the default converter will not be able to deal with Kotlin objects for example.

I see that Maxim commented the following in #468: "But the format of the search attribute serialization is fixed by the service."
Are we strictly limited by what can be a search attribute and these classes are provided by Temporal + Java SDK and all of them are final?

-----UPDATE------
Answering my own question (from here https://docs.temporal.io/docs/server/workflow-search/):

Search attributes must be one of the following types:
string
keyword
int
double
bool
datetime

So we should be good with using the default converter. But a nice long comment that is explaining this would be really useful in the code about why do we treat search attributes differently.

@vitarb
Copy link
Contributor Author

vitarb commented May 11, 2021

So it looks like we are in agreement that we should make this change, let me brush it up a bit then.

@vitarb vitarb force-pushed the use-default-conv-for-sa branch from 9a59721 to 1d756e0 Compare May 12, 2021 01:56
@vitarb vitarb merged commit 192f0c4 into temporalio:master May 12, 2021
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.

4 participants