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

Remove search attribute registration before starting every workflow #94

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

bergundy
Copy link
Member

Omes is supposed to replicate user behavior to generate load on the server. Registering search attributes doesn't belong in the hot path, it should be done as a separate setup step.

See also: #93

return fmt.Errorf("failed to register search attributes: %w", err)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have the necessary context here. This was added by @Sushisource in #41. Is it not needed for anything?

Copy link
Contributor

@dandavison dandavison Jul 17, 2024

Choose a reason for hiding this comment

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

Discussed internally. The plan is for applications using omes (such as CI/CD pipelines) to do this.

Copy link
Member

Choose a reason for hiding this comment

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

We still need some way for this to happen automatically so that when developers come along and want to just run omes than they don't have to interpret why things are failing.

This should be automatically done somewhere in this codebase still. It doesn't have to be here, but I don't want to merge this PR without doing it somewhere else, or, at the bare minimum, documenting the necessity

Copy link
Member Author

Choose a reason for hiding this comment

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

That works for me.

Copy link
Member

Choose a reason for hiding this comment

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

wait, but, you've just merged it without those things 😅

Copy link
Member

@cretz cretz Jul 17, 2024

Choose a reason for hiding this comment

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

Yeah, I was hoping that at least the scenario requirements would be documented (though would prefer a bootstrapping command). Having an onerous environment setup process harms these kinds of tools.

@bergundy bergundy merged commit 2f4364b into temporalio:main Jul 17, 2024
9 checks passed
@bergundy bergundy deleted the no-search-attributes-reg branch July 17, 2024 17:29
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.

None yet

4 participants