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

Operator API #2503

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Operator API #2503

merged 1 commit into from
Feb 24, 2022

Conversation

Spikhalskiy
Copy link
Contributor

No description provided.

client/frontend/client.go Outdated Show resolved Hide resolved
@Spikhalskiy Spikhalskiy force-pushed the operator-api branch 3 times, most recently from d26349e to 5a72a3b Compare February 16, 2022 05:12
Copy link
Member

@alexshtin alexshtin left a comment

Choose a reason for hiding this comment

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

Most of the handler code is copied from AdminHandler, right? And you gonna keep it there for some period of time. It would be great to reduce code duplication and have an instance of OpearatorHandler in AdminHandeler and call these 3 methods from it. With proper comment that it is deprecated and so on.

service/frontend/operatorHandler.go Outdated Show resolved Hide resolved
service/frontend/operatorHandler.go Outdated Show resolved Hide resolved
@Spikhalskiy Spikhalskiy force-pushed the operator-api branch 2 times, most recently from 0e6bc34 to 730763e Compare February 21, 2022 20:14
@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Feb 21, 2022

@alexshtin I would generally agree with you about code duplication if there are two different active endpoints that are using the same code.
But here the situation is a bit different, we will have a deprecated and a new version of the endpoints and none of them by themselves is a superset. And we actually will not need to keep the old endpoint updated with the new functionality.
I think we should go copy-paste route in this situation because it will SIMPLIFY support and REDUCE the burden. Your approach while having a good intention to dedup the code will likely bring more complexity and potential mistakes.

The way you propose to directly use operator_handler in admin_handler will not fly, because operator_handler doesn't provide a superset of operations/return values, the endpoints are slightly different. For example, the new operator endpoint doesn't return some of the data that is returned by the deprecated version (GetSearchAttributesResponse#runId as an example).
Striving to remove code duplication here will have to create of a shared manager that is used by old and new endpoints sets and provides a superset of functionality.

This way, every time we will be editing the new version of the code you will have to worry about not breaking and supporting the code paths used by the deprecated version. Compare it with the current approach, where we will be just directly editing the new endpoints and can forget about updating the old ones.
If we dedup way, we also can't just remove the deprecated endpoints in the future. We will need to 1. remove unused functionality from the shared code 2. move the shared code to the operator handler.

@Spikhalskiy Spikhalskiy force-pushed the operator-api branch 4 times, most recently from e690b80 to bd59d52 Compare February 22, 2022 22:48
@alexshtin
Copy link
Member

I would agree on this. Let's leave two copies.

@Spikhalskiy Spikhalskiy force-pushed the operator-api branch 4 times, most recently from e491aef to 642ffc9 Compare February 23, 2022 05:01
@Spikhalskiy Spikhalskiy marked this pull request as ready for review February 23, 2022 05:44
@Spikhalskiy Spikhalskiy requested a review from a team as a code owner February 23, 2022 05:44
Operator Service with Search Attributes
@Spikhalskiy Spikhalskiy merged commit 4d129d1 into temporalio:master Feb 24, 2022
@Spikhalskiy Spikhalskiy deleted the operator-api branch April 21, 2022 01:24
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