-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Remove LegacyCTRAL from AutoCreateAction #84170
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 LegacyCTRAL from AutoCreateAction #84170
Conversation
999f845 to
7e2ff44
Compare
|
Pinging @elastic/es-data-management (Team:Data Management) |
| }; | ||
| } | ||
|
|
||
| ClusterState execute( |
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.
Is it worth a comment on the implied contract w.r.t. returning a clusterState, calling success/failure on the taskContext, and updating successfulRequests? It looks to me like you're doing all the juggling just fine, but some future developer might be happy to add a new return case and think that just satisfying the compiler here is sufficient.
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.
Yes, good point. I added a comment in 6be0514 and also an assertion to enforce it.
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.
LGTM, I'd like to see a comment about it, but I'm not going to block on that.
Relates #83784