Remove AcknowledgedRestListener in favour of RestToXContentListener #28724
Conversation
This commit makes AcknowledgedResponse implement ToXContentObject, so that the response knows how to print its own content out to XContent, which allows us to remove AcknowledgedRestListener.
javanna
added
>enhancement
review
:Core/Infra/REST API
|
|
||
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * Response returned after a cluster reroute request | ||
| */ | ||
| public class ClusterRerouteResponse extends AcknowledgedResponse { | ||
| public class ClusterRerouteResponse extends AcknowledgedResponse implements ToXContentObject { |
There was a problem hiding this comment.
I'm curious why this class didn't implement toXContent before. Was it not client-facing, that is not used in the rest API?
There was a problem hiding this comment.
because the API is not supported yet by the high-level REST client
| return builder; | ||
| } | ||
|
|
||
| protected void addCustomFields(XContentBuilder builder, Params params) throws IOException { |
There was a problem hiding this comment.
Wondering if this could be abstract, so subclasses don't forget to implement it. Looks like many would have an empty implementation though, so just a thought.
There was a problem hiding this comment.
it could be, but the classes that have a non-empty impl for it are more of an exception, so maybe it's ok like this?
| @Override | ||
| protected void addCustomFields(XContentBuilder builder, Params params) throws IOException { | ||
| builder.startObject("state"); | ||
| state.toXContent(builder, params); |
There was a problem hiding this comment.
Okay, I see this was previously done in the RestClusterRerouteAction.
| protected void addCustomFields(XContentBuilder builder, ClusterRerouteResponse response) throws IOException { | ||
| builder.startObject("state"); | ||
| settingsFilter.addFilterSettingParams(request); | ||
| response.getState().toXContent(builder, request); |
There was a problem hiding this comment.
I see that applying the settingsFilter moved up in this method, but how do we know that the request (as Params) is what is used then rendering the clusterstate now in the response? I was following the code but couldn't connect the dots yet, maybe you know?
There was a problem hiding this comment.
good one, I think it works as I don't think I changed anything substantial, but I meant to add a test for this and I forgot, thanks for reminding me ;)
There was a problem hiding this comment.
I think that this is what you're looking for: https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/rest/action/RestToXContentListener.java#L47 . The channel.request gets passed in as params to the toXContent call
There was a problem hiding this comment.
Thanks for checking, yes, that looks like how it works. Adding a test wouldn't hurt, but might not me really necessary either. I'm fine with this explanation.
|
retest this please |
…28724) This commit makes AcknowledgedResponse implement ToXContentObject, so that the response knows how to print its own content out to XContent, which allows us to remove AcknowledgedRestListener.
* es/master: (143 commits) Revert "Disable BWC tests for build issues" Remove AcknowledgedRestListener in favour of RestToXContentListener (#28724) Build: Consolidate archives and packages configuration (#28760) Skip some plugins service tests on Windows Migrate some *ResponseTests to AbstractStreamableXContentTestCase (#28749) Disable BWC tests for build issues Ensure that azure stream has socket privileges (#28751) [DOCS] Fixed broken link. Pass InputStream when creating XContent parser (#28754) [DOCS] Changed to use transient setting to reenabled allocation. Closes #27677 Delay path expansion on Windows [TEST] replace randomAsciiAlphanumOfLengthBetween with randomAsciiLettersOfLengthBetween [Tests] Extract the testing logic for Google Cloud Storage (#28576) [Docs] Update links to java9 docs (#28750) version set in ingest pipeline (#27573) Revert "Add startup logging for standalone tests" Tests: don't wait for completion while trying to get completed task Add 5.6.9 snapshot version [Docs] Java high-level REST client : clean up (#28703) Updated distribution outputs in contributing docs ...
* es/6.x: (140 commits) Remove AcknowledgedRestListener in favour of RestToXContentListener (#28724) Migrate some *ResponseTests to AbstractStreamableXContentTestCase (#28749) Revert "Disable BWC tests for build issues" Skip some plugins service tests on Windows Build: Consolidate archives and packages configuration (#28760) Ensure that azure stream has socket privileges (#28773) Disable BWC tests for build issues Pass InputStream when creating XContent parser (#28754) [DOCS] Fixed broken link. [DOCS] Changed to use transient setting to reenabled allocation. Closes #27677 Delay path expansion on Windows [TEST] replace randomAsciiAlphanumOfLengthBetween with randomAsciiLettersOfLengthBetween REST high-level client: add support for Rollover Index API (#28698) [Tests] Extract the testing logic for Google Cloud Storage (#28576) Moved Grok helper code to a separate Gradle module and let ingest-common module depend on it. version set in ingest pipeline (#27573) [Docs] Update links to java9 docs (#28750) Revert "Add startup logging for standalone tests" Tests: don't wait for completion while trying to get completed task Add 5.6.9 snapshot version ...
…lastic#28724) This commit makes AcknowledgedResponse implement ToXContentObject, so that the response knows how to print its own content out to XContent, which allows us to remove AcknowledgedRestListener.
This commit makes
AcknowledgedResponseimplementToXContentObject, so that the response knows how to print its own content out toXContent, which allows us to removeAcknowledgedRestListener.Thanks to the recent high-level REST client effort, the only response that extends
AcknowledgedResponseand needs to useAcknowledgedRestListenerisClusterRerouteResponse, but it can easily be fixed by moving it's rendering code to the response class.Relates to #3889