Remove LegacyCTRAListener from MetadataUpdateSettingsService #84985
Conversation
arteam
added
>non-issue
>refactoring
:Distributed Coordination/Cluster Coordination
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
I would prefer we clean up this class's task/executor as part of this change, bring it more in line with how we think these things should be implemented. That's really the purpose of the LegacyClusterTaskResultActionListener marker.
E.g. the task itself should mostly just be data (typically a record, and definitely not a subclass of AckedClusterStateUpdateTask) and the execution logic moved to the executor.
Ideally we'd not build a new cluster state each time round the loop too, although I tried that with this task and it seemed pretty tricky because of how we update the Metadata.Builder before we've finished doing all the validation steps. I can live without that change here, but we should definitely do more cleanup than just this one thing.
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java
Outdated
Show resolved
Hide resolved
…taUpdateSettingsService.java Co-authored-by: David Turner <[email protected]>
…a/MetadataUpdateSettingsService.java" This reverts commit f8709a0.
|
@elasticmachine update branch |
arteam
force-pushed
the
remove-lcrta-from-metadata-update-setting-service
branch
from
2b2205e to
9152a80
Compare
|
@elasticmachine run elasticsearch-ci/bwc |
|
@elasticmachine update branch |
There was a problem hiding this comment.
Looks good.
I now realise that we didn't add tests when we made this API implement batching. I expect any problems would trip something somewhere but it would be good to have a test suite that focusses on this. See MetadataIndexStateServiceBatchingTests for some ideas about how to write this kind of test.
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public void updateSettings(final UpdateSettingsClusterStateUpdateRequest request, final ActionListener<AcknowledgedResponse> listener) { | ||
| ActionListener<AcknowledgedResponse> actionListener = wrapPreservingContext(listener, threadPool.getThreadContext()); |
There was a problem hiding this comment.
Hmm I think this wrapping is very old and hasn't been necessary for ages. Could you add an ESSingleNodeTestCase test which sets up a thread context, calls the update settings API, and asserts that the context is passed through to the listener (even without this wrapping). See e.g. how MasterServiceTests#testTaskFailureNotification interacts with the thread context.
|
Sure, let me add a test! |
…taUpdateSettingsService.java Co-authored-by: David Turner <[email protected]>
…taUpdateSettingsService.java Co-authored-by: David Turner <[email protected]>
See #83784