Centralize codes related to 'master_timeout' deprecation for eaiser removal - in CAT Nodes API #2670
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| /* | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * The OpenSearch Contributors require contributions made to | ||
| * this file be licensed under the Apache-2.0 license or a | ||
| * compatible open source license. | ||
| */ | ||
|
|
||
| package org.opensearch.action; | ||
|
|
||
| import org.junit.AfterClass; | ||
| import org.opensearch.OpenSearchParseException; | ||
| import org.opensearch.client.node.NodeClient; | ||
| import org.opensearch.common.settings.Settings; | ||
| import org.opensearch.rest.action.cat.RestNodesAction; | ||
| import org.opensearch.test.OpenSearchTestCase; | ||
| import org.opensearch.test.rest.FakeRestRequest; | ||
| import org.opensearch.threadpool.TestThreadPool; | ||
|
|
||
| import static org.hamcrest.Matchers.containsString; | ||
|
|
||
| /** | ||
| * As of 2.0, the request parameter 'master_timeout' in all applicable REST APIs is deprecated, | ||
| * and alternative parameter 'cluster_manager_timeout' is added. | ||
| * The tests are used to validate the behavior about the renamed request parameter. | ||
| * Remove the test after removing MASTER_ROLE and 'master_timeout'. | ||
| */ | ||
| public class RenamedTimeoutRequestParameterTests extends OpenSearchTestCase { | ||
| private static final TestThreadPool threadPool = new TestThreadPool(RenamedTimeoutRequestParameterTests.class.getName()); | ||
| private final NodeClient client = new NodeClient(Settings.EMPTY, threadPool); | ||
|
|
||
| private static final String DUPLICATE_PARAMETER_ERROR_MESSAGE = | ||
| "Please only use one of the request parameters [master_timeout, cluster_manager_timeout]."; | ||
| private static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE = | ||
| "Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version."; | ||
|
|
||
| @AfterClass | ||
| public static void terminateThreadPool() { | ||
| terminate(threadPool); | ||
| } | ||
|
|
||
| /** | ||
| * Validate both parameters 'cluster_manager_timeout' and its predecessor can be parsed correctly. | ||
| */ | ||
| public void testCatAllocation() { | ||
|
There was a problem hiding this comment. You should probably break this into 3 separate test methods, something like:
Then you don't need the comments because the test methods are self-explanatory, and if there was any failure it would point to a more specific cause. There was a problem hiding this comment.
Haha, looks like I should explicitly state do not adding new tests to this class, I'm joking. Though it is a temporary and tedious test case. There was a problem hiding this comment. Well, the method There was a problem hiding this comment. I'm just suggesting that you break this test method down into 3 separate tests instead of 1, since it is actually testing 3 separate things. I think it's fine to test this behavior in this one class and was not suggesting that you add tests to all other APIs. There was a problem hiding this comment. I see 😄. I agree to test the behavior in this class, and I will modify the unit tests. There was a problem hiding this comment. Thanks for your suggestions! I modified to test one behavior in a single test case. For the other REST APIs that has got "master_timeout" parameter, I will add one test case for each. |
||
| RestNodesAction action = new RestNodesAction(); | ||
| // Request with only new parameter will be parsed without warning and exception. | ||
| action.doCatRequest(getRestRequestWithNewParam(), client); | ||
| // Request with only deprecated parameter will result deprecation warning. | ||
| action.doCatRequest(getRestRequestWithDeprecatedParam(), client); | ||
| assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE); | ||
| // Request with both new and deprecated parameters and different values will result exception. | ||
| // It should have warning, but the same deprecation warning won't be logged again. | ||
| Exception e = assertThrows(OpenSearchParseException.class, () -> action.doCatRequest(getRestRequestWithWrongValues(), client)); | ||
| assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE)); | ||
| } | ||
|
|
||
| private FakeRestRequest getRestRequestWithWrongValues() { | ||
| FakeRestRequest request = new FakeRestRequest(); | ||
| request.params().put("cluster_manager_timeout", "1h"); | ||
| request.params().put("master_timeout", "3s"); | ||
| return request; | ||
| } | ||
|
|
||
| private FakeRestRequest getRestRequestWithDeprecatedParam() { | ||
| FakeRestRequest request = new FakeRestRequest(); | ||
| request.params().put("master_timeout", "3s"); | ||
| return request; | ||
| } | ||
|
|
||
| private FakeRestRequest getRestRequestWithNewParam() { | ||
| FakeRestRequest request = new FakeRestRequest(); | ||
| request.params().put("cluster_manager_timeout", "2m"); | ||
| return request; | ||
| } | ||
| } | ||
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.
Does this have to be static?
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.
Thanks for this suggestion! I will learn from it.
It doesn't, the variable can has got any value as long as it's an instance of the same class.
The reason I use static is just usually heard others saying "to define a constant, use
static final".I will remove
statickeyword.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.
Ah, because the method with
@AfterClassannotation has to bestatic, and I wrote a method to terminate the ThreadPool after every test in Line 37, I can only make thethreadPoolvariablestatic.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.
Yeah, the issue here is that a thread pool can't really be a "constant" because it is a stateful and mutable object (hence the need to terminate it). For any scalar values or immutable objects it would be fine to make them
static finalconstants.A potentially valid reason for making the thread pool static would be that if it was expensive to create and tear down, so you only wanted to do it once and run multiple tests with it. That carries the risk of tests interfering with one another since they would share mutable static state, so that should only be done in cases where it makes a big difference in test run time.
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.
Just use the
@Afterannotation on a non-static method and it will be run after every test case.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.
Thank you so much for telling me the detail! 👍 👍 (String is immutable, so it's fine to be static 😄)
Sorry my mistake, I wanted to say the
threadPoolcan be terminated after all test cases run and can be shared with test cases, because I think the interfering is not a problem, because my test cases only care about the request parameters, and doesn't check the result.So looks fine to keep the threadpool it static.
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.
You're right that the current implementation will likely work fine with a static thread pool. But in absence of a strong reason to use shared static state, you should prefer to avoid shared static state. You never know what kind of test the next developer will add to this test class :)
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.
Ok, I updated the threadpool to be non-static, and terminate the threadpool after every test cases, hope it will not expensive to tear down. 😄 Thank you!