[Monitoring] Remove `include_type_name` parameter from GET _template request by ycombinator · Pull Request #38818 · elastic/elasticsearch

ycombinator · 2019-02-12T21:04:51Z

The HTTP exporter code in the Monitoring plugin makes GET _template requests to check for existence of templates. These requests don't need to pass the include_type_name query parameter so this PR removes it from the request. This should remove the following deprecation log entries on the Monitoring cluster in 7.0.0 onwards:

[types removal] Specifying include_type_name in get index template requests is deprecated.

Addresses #37442.

elasticmachine · 2019-02-12T21:11:59Z

Pinging @elastic/es-core-features

jtibshirani

Thanks @ycombinator! I left some very minor comments.

jtibshirani · 2019-02-12T21:11:05Z

.../src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java

     * @param logger The logger to use for status messages.
     * @param resourceBasePath The base path/endpoint to check for the resource (e.g., "/_template").
     * @param resourceName The name of the resource (e.g., "template123").
+     * @param parameters Map of query string parametrs, if any.


parametrs -> parameters

jtibshirani · 2019-02-12T21:17:15Z

...ing/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/TemplateHttpResource.java

     */
    @Override
    protected void doPublish(final RestClient client, final ActionListener<Boolean> listener) {
+        Map<String, String> parameters = new TreeMap<>();


Really small comment, I think this could just be Map<String, String> parameters = Collections.singletonMap(INCLUDE_TYPE_NAME_PARAMETER, "true");.

jtibshirani · 2019-02-12T21:20:24Z

.../src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java

        final Request request = new Request("PUT", resourceBasePath + "/" + resourceName);
-        addParameters(request);
+        addDefaultParameters(request);
+        if (parameters != null) {


I tend to prefer using empty collections instead of making parameters nullable (we could just use Collections.emptyMap() in all the calls that don't need additional parameters).

.../src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java

jakelandis · 2019-02-12T23:15:36Z

.../src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java


-    private void addParameters(final Request request) {
+    private void addDefaultParameters(final Request request) {
+        this.addParameters(request, this.parameters);


nitpick: avoid using this. except in constructors and equals methods.

jakelandis

LGTM

pickypg

Change LGTM. Minor nitpicks for whitespace changes that make it look like other parts changed.

pickypg · 2019-02-13T00:22:47Z

...ing/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/TemplateHttpResource.java

    protected void doCheck(final RestClient client, final ActionListener<Boolean> listener) {
        versionCheckForResource(client, listener, logger,
-                                "/_template", templateName, "monitoring template",
+                                "/_template", templateName,  "monitoring template",


Nitpick: unneeded white space change.

pickypg · 2019-02-13T00:22:55Z

...rc/main/java/org/elasticsearch/xpack/monitoring/exporter/http/WatcherExistsHttpResource.java


        checkForResource(client, listener, logger,
-                         "", "_xpack", "watcher check",
+                         "", "_xpack","watcher check",


Nitpick: unneeded white space change.

ycombinator · 2019-02-14T01:04:59Z

@jtibshirani @jakelandis @pickypg I made a few more changes, primarily to tests, after your last rounds of review. So would you mind taking another gander at this PR when you get a chance? Thanks!

jtibshirani

The new changes look good to me.

jtibshirani · 2019-02-14T01:13:57Z

...onitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterIT.java

                assertThat(putRequest.getMethod(), equalTo("PUT"));
                assertThat(putRequest.getUri().getPath(), equalTo(pathPrefix + resourcePrefix + resource.v1()));
-                assertMonitorVersionQueryString(resourcePrefix, getRequest.getUri().getQuery());
+                final String[] parameters = resourcePrefix.startsWith("/_template")


Small comment, but but maybe it could be more natural to use a Map here?

jakelandis · 2019-02-14T17:42:41Z

...onitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterIT.java

+            completeQueryString = queryString + "&" + completeQueryString;
        }
+
+        assertThat(query, equalTo(completeQueryString));


I believe that this assert will only work on with empty or single params, or maps backed by a TreeMap since the order is relevant when comparing the String representation.

I would suggest to avoid re-building the string, and run the String result of getRequest.getUri().getQuery() (e.g. the first param) through https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/rest/RestUtils.java#L40 to get the corresponding Map and then compare the Maps.

jakelandis · 2019-02-14T17:55:27Z

...rg/elasticsearch/xpack/monitoring/exporter/http/AbstractPublishableHttpResourceTestCase.java


        verifyListener(null);

+        Map <String, String> allParameters = new TreeMap<>();


nitpick: Generally try to avoid TreeMap unless order is important.

jakelandis

LGTM

…request (elastic#38818) The HTTP exporter code in the Monitoring plugin makes `GET _template` requests to check for existence of templates. These requests don't need to pass the `include_type_name` query parameter so this PR removes it from the request. This should remove the following deprecation log entries on the Monitoring cluster in 7.0.0 onwards: ``` [types removal] Specifying include_type_name in get index template requests is deprecated. ```

* master: Adjust BWC version for token/API key service (elastic#38917) [Monitoring] Remove `include_type_name` parameter from GET _template request (elastic#38818) Revert "[test] disable packaging tests for suse boxes" (elastic#38864)

…request (#38926) Backport of #38818 to `7.0`. Original description: The HTTP exporter code in the Monitoring plugin makes `GET _template` requests to check for existence of templates. These requests don't need to pass the `include_type_name` query parameter so this PR removes it from the request. This should remove the following deprecation log entries on the Monitoring cluster in 7.0.0 onwards: ``` [types removal] Specifying include_type_name in get index template requests is deprecated. ```

…request (#38925) Backport of #38818 to `7.x`. Original description: The HTTP exporter code in the Monitoring plugin makes `GET _template` requests to check for existence of templates. These requests don't need to pass the `include_type_name` query parameter so this PR removes it from the request. This should remove the following deprecation log entries on the Monitoring cluster in 7.0.0 onwards: ``` [types removal] Specifying include_type_name in get index template requests is deprecated. ```

Remove include_type_name parameter from GET _template request

a5caae1

ycombinator added v7.0.0 backport pending v8.0.0 v7.2.0 labels Feb 12, 2019

ycombinator requested review from jakelandis, jtibshirani and pickypg February 12, 2019 21:04

ycombinator added the :Core/Infra/Monitoring DEPRECATED, DO NOT USE label Feb 12, 2019

jtibshirani reviewed Feb 12, 2019

View reviewed changes

Fixing typo

087d982

jakelandis reviewed Feb 12, 2019

View reviewed changes

.../src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java Show resolved Hide resolved

jakelandis reviewed Feb 12, 2019

View reviewed changes

ycombinator added 4 commits February 12, 2019 15:40

Use Collections.emptyMap() instead of null

bd01209

Change class-level variable name to defaultParameters

7c54821

Rename method: getParameters -> getDefaultParameters

47478ce

Simplifying code

6c8e477

jakelandis approved these changes Feb 12, 2019

View reviewed changes

jtibshirani approved these changes Feb 12, 2019

View reviewed changes

Undoing unnecessary changes made my mistake

814a838

pickypg approved these changes Feb 13, 2019

View reviewed changes

ycombinator added 8 commits February 12, 2019 16:32

Undoing unnecessary whitespace changes

d7b9bea

Fixing checkstyle violations

d2511d9

Removing unnecessary change

d93d990

Forgot to remove the parameter from _all_ template requests 🤦

ce558c6

Updating tests

175440f

Fixing map operation

b5a126f

Integration test updates

5759914

Fixing checkstyle violations

9b9853d

ycombinator added 2 commits February 13, 2019 13:25

Only expect include_type_name for PUT template requests

fda3c17

Fixing checkstyle violations

0f1bc7e

jtibshirani approved these changes Feb 14, 2019

View reviewed changes

ycombinator added 2 commits February 13, 2019 17:56

Switching parameters to Map

ebda96d

Fixing logic

1d8a0c1

jakelandis reviewed Feb 14, 2019

View reviewed changes

ycombinator added 2 commits February 14, 2019 10:28

Use HashMap instead of TreeMap

baf4fd1

Compare maps not strings so ordering won't be significant

7ca71c6

jakelandis approved these changes Feb 14, 2019

View reviewed changes

ycombinator merged commit 7bcfa45 into elastic:master Feb 14, 2019

ycombinator deleted the monitoring-remove-type-param branch February 14, 2019 20:55

ycombinator mentioned this pull request Feb 14, 2019

[Monitoring] Remove include_type_name parameter from GET _template request #38925

Merged

ycombinator mentioned this pull request Feb 14, 2019

[Monitoring] Remove include_type_name parameter from GET _template request #38926

Merged

ycombinator removed the backport pending label Feb 20, 2019

michaelbaamonde added v7.0.0-rc1 and removed v7.0.0 labels Mar 25, 2019

colings86 added the >non-issue label Jun 18, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021


		verifyListener(null);

		Map <String, String> allParameters = new TreeMap<>();

Conversation

ycombinator commented Feb 12, 2019 • edited by jtibshirani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 12, 2019

Uh oh!

jtibshirani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

pickypg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator commented Feb 14, 2019

Uh oh!

jtibshirani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ycombinator commented Feb 12, 2019 •

edited by jtibshirani

Loading