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

Fix elastic search index name for ExecutionStatus #544

Merged

Conversation

samarabbas
Copy link
Contributor

What changed?
Update index key definition for Elastic Search in development_es.yaml to reflect the recent renaming from
Status to ExecutionStatus.

Why?
Recently we did massive renaming of ElasticSearch indexes. As part of this renaming Status index
created by the system to represent status of closed workflow execution was renamed to
ExecutionStatus. Default in the code is updated to represent this change but dynamic config
override used by local development with ES support was not updated. This resulted in indexes to
go out of sync in all places which relies on development_es.yaml dynamic config file. This broke
local development setup and anyone using docker-compose_es.yaml.

How did you test it?
Locally run Temporal server using dynamic config overrides in development_es.yaml. Made sure
visibility is working after execution closes.

Potential risks
This should only impact local development setups with Elastic Search.

Copy link
Member

@mastermanu mastermanu left a comment

Choose a reason for hiding this comment

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

can you also update schema/elasticsearch/visibility/index_template.json

update: never-mind, this looks mostly in-sync to me. couple of minor comments about mismatched types

@@ -13,7 +13,7 @@ frontend.validSearchAttributes:
StartTime: "Int"
Copy link
Member

@mastermanu mastermanu Jul 15, 2020

Choose a reason for hiding this comment

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

the index_template.json has this as a long. Same with executiontime and closetime and customintfield

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like proto definition only defines the following four types:

message Field {
    temporal.server.api.enums.v1.FieldType type = 1;
    oneof data {
        string string_data = 2;
        int64 int_data = 3;
        bool bool_data = 4;
        bytes binary_data = 5;
    }
}

Copy link
Member

@mastermanu mastermanu Jul 15, 2020

Choose a reason for hiding this comment

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

hmm, then maybe we should just fix up index_template to use integer then (or maybe switch all of them to long if proto is int64)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@samarabbas samarabbas force-pushed the fix-index-key-for-elastic-search branch from 4a31161 to 9e7e382 Compare July 15, 2020 19:10
@samarabbas samarabbas merged commit eaf9ee9 into temporalio:master Jul 15, 2020
@samarabbas samarabbas deleted the fix-index-key-for-elastic-search branch July 15, 2020 21:39
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