Skip to content
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

Make High-Level-Rest-Client tests allow deprecation warning temporarily, during deprecation of request parameter 'master_timeout' #2702

Merged
merged 2 commits into from
Apr 4, 2022

Conversation

tlfeng
Copy link
Collaborator

@tlfeng tlfeng commented Apr 1, 2022

Description

Change:

  • Make Rest-High-Rest-Level tests allow deprecation warning temporarily, by changing the argument of setStrictDeprecationMode() to false when building RestClient for tests.

Reason:
Many High-Level-Rest-Client tests are broken due to deprecation warning, during the process of deprecating master_timeout parameter of REST APIs.
The simplest solution is:

  1. Set "StrictDeprecationMode" to false to make tests allow deprecation warning.
  2. Add alternative parameter cluster_manager_timeout for all applicable REST APIs
  3. Change to use cluster_manager_timeout instead of master_timeout in High-Level-Rest-Client RequestConverter class
  4. Set "StrictDeprecationMode" back to true

Note:

A Sample test failure

> Task :client:rest-high-level:asyncIntegTest

REPRODUCE WITH: ./gradlew ':client:rest-high-level:asyncIntegTest' --tests "org.opensearch.client.ClusterClientIT.testClusterGetSettings" -Dtests.seed=F5E30873F5E8B3F6 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=sr-Latn-BA -Dtests.timezone=America/Santa_Isabel -Druntime.java=17

org.opensearch.client.ClusterClientIT > testClusterGetSettings FAILED
    org.opensearch.client.WarningFailureException: method [GET], host [http://127.0.0.1:38451], URI [/_cluster/settings?master_timeout=30s], status line [HTTP/1.1 200 OK]
    Warnings: [Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version.]
    {"persistent":{"cluster":{"routing":{"allocation":{"enable":"NONE"}}}},"transient":{"indices":{"recovery":{"max_bytes_per_sec":"10b"}}}}
        at __randomizedtesting.SeedInfo.seed([F5E30873F5E8B3F6:8CF8FB5CACB76F2B]:0)
        at app//org.opensearch.client.RestClient.convertResponse(RestClient.java:346)

Issues Resolved

A part of issue #2511

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@tlfeng tlfeng added :test Adding or fixing a test v2.0.0 Version 2.0.0 backport 2.x Backport to 2.x branch backport 2.0 Backport to 2.0 branch labels Apr 1, 2022
@tlfeng tlfeng changed the title Mute High-Level-Rest-Client tests that using 'master_timeout' parameter temporarily Mute High-Level-Rest-Client tests that REST API requests with 'master_timeout' parameter will be sent temporarily Apr 1, 2022
@tlfeng tlfeng changed the title Mute High-Level-Rest-Client tests that REST API requests with 'master_timeout' parameter will be sent temporarily Mute High-Level-Rest-Client tests where REST API requests with 'master_timeout' parameter will be sent temporarily Apr 1, 2022
@tlfeng tlfeng marked this pull request as ready for review April 1, 2022 09:12
@tlfeng tlfeng requested a review from a team as a code owner April 1, 2022 09:12
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 3bda2c9
Log 4026

Reports 4026

@@ -84,6 +85,7 @@
import static org.hamcrest.Matchers.lessThan;
import static org.hamcrest.Matchers.notNullValue;

@LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/pull/2683")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for raising this PR.

I have one concern that muting test classes has potential of introducing bugs from other code changes exercising these two test classes. How do we have plan to cover this ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dreamer-89 thank you for reviewing my PR.
My thought is to hold merging this PR until all the other PRs are ready to be merged in #2511. When getting ready, then merging all those PRs together in a short time. The potential of other code changes merged can be reduced by try to shorten the interval of muting the tests.
This is a good point, I should put up some notice saying not to merge this PR in a hurry. 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tlfeng for clarifying.

Is it possible we can add OpenSearchRestTestCase.expectWarnings in tests in above two tests files wherever we are adding master_timeout. I think this is how we are tackling other deprecated features on client side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your good suggestion!
You are right. 👍 Looks like I should turn off the enableWarningsCheck https://github.com/opensearch-project/OpenSearch/blob/1.3.1/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java#L407 for the affected test classes.
There are 24 classes extends this class OpenSearchRestHighLevelClientTestCase (and one class has turned it off https://github.com/opensearch-project/OpenSearch/blob/1.3.1/client/rest-high-level/src/test/java/org/opensearch/client/BulkProcessorIT.java#L89) so the amount of work is reasonable.
I will apply it to the affected test classes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the code. The enableWarningsCheck doesn't work for this case, but instead, I found using setStrictDeprecationMode(false) to build the RestClient can make the test allow deprecation warnings. This will be a safer way 😄.

…cation warning in HLRC tests

Signed-off-by: Tianli Feng <[email protected]>
@tlfeng tlfeng force-pushed the mute-hlrc-test-timeout branch from a88317b to 673e55b Compare April 2, 2022 01:44
@tlfeng tlfeng changed the title Mute High-Level-Rest-Client tests where REST API requests with 'master_timeout' parameter will be sent temporarily Make Rest-High-Rest-Level tests allow deprecation warning temporarily Apr 2, 2022
@tlfeng tlfeng changed the title Make Rest-High-Rest-Level tests allow deprecation warning temporarily Make Rest-High-Rest-Level tests allow deprecation warning temporarily during deprecation of request parameter 'master_timeout' Apr 2, 2022
@tlfeng tlfeng changed the title Make Rest-High-Rest-Level tests allow deprecation warning temporarily during deprecation of request parameter 'master_timeout' Make Rest-High-Rest-Level tests allow deprecation warning temporarily, during deprecation of request parameter 'master_timeout' Apr 2, 2022
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 673e55b
Log 4064

Reports 4064

Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM...hopefully this won't stay in flight too long.

@nknize nknize merged commit 6a2a33d into opensearch-project:main Apr 4, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 4, 2022
…, during deprecation of request parameter 'master_timeout' (#2702)

Temporarily build rest client with setStrictDeprecationMode(false) to allow
deprecation warning in HLRC tests while master_timeout parameters is
being refactored.

Signed-off-by: Tianli Feng <[email protected]>
(cherry picked from commit 6a2a33d)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 4, 2022
…, during deprecation of request parameter 'master_timeout' (#2702)

Temporarily build rest client with setStrictDeprecationMode(false) to allow
deprecation warning in HLRC tests while master_timeout parameters is
being refactored.

Signed-off-by: Tianli Feng <[email protected]>
(cherry picked from commit 6a2a33d)
@tlfeng
Copy link
Collaborator Author

tlfeng commented Apr 4, 2022

LGTM...hopefully this won't stay in flight too long.

@nknize Thanks for reviewing my PR! 👍 I will try to merging other PRs in #2511 soon and revert this change in this week.

nknize pushed a commit that referenced this pull request Apr 4, 2022
…, during deprecation of request parameter 'master_timeout' (#2702) (#2740)

Temporarily build rest client with setStrictDeprecationMode(false) to allow
deprecation warning in HLRC tests while master_timeout parameters is
being refactored.

Signed-off-by: Tianli Feng <[email protected]>
(cherry picked from commit 6a2a33d)
nknize pushed a commit that referenced this pull request Apr 4, 2022
…, during deprecation of request parameter 'master_timeout' (#2702) (#2741)

Temporarily build rest client with setStrictDeprecationMode(false) to allow
deprecation warning in HLRC tests while master_timeout parameters is
being refactored.

Signed-off-by: Tianli Feng <[email protected]>
(cherry picked from commit 6a2a33d)
@tlfeng tlfeng changed the title Make Rest-High-Rest-Level tests allow deprecation warning temporarily, during deprecation of request parameter 'master_timeout' Make High-Level-Rest-Client tests allow deprecation warning temporarily, during deprecation of request parameter 'master_timeout' Apr 4, 2022
tlfeng pushed a commit to tlfeng/OpenSearch that referenced this pull request Apr 4, 2022
…emporarily, during deprecation of request parameter 'master_timeout' (opensearch-project#2702)"

This reverts commit 6a2a33d.

Signed-off-by: Tianli Feng <[email protected]>
@tlfeng tlfeng deleted the mute-hlrc-test-timeout branch April 5, 2022 06:20
tlfeng pushed a commit to tlfeng/OpenSearch that referenced this pull request Apr 15, 2022
…emporarily, during deprecation of request parameter 'master_timeout' (opensearch-project#2702)"

This reverts commit 6a2a33d.

Signed-off-by: Tianli Feng <[email protected]>
tlfeng pushed a commit that referenced this pull request Apr 16, 2022
…emporarily, during deprecation of request parameter 'master_timeout' (#2702)" (#2930)

This reverts commit 6a2a33d.
* Sets the High-Level-Rest-Client tests back to treating warning header as a failure.

Signed-off-by: Tianli Feng <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 16, 2022
…emporarily, during deprecation of request parameter 'master_timeout' (#2702)" (#2930)

This reverts commit 6a2a33d.
* Sets the High-Level-Rest-Client tests back to treating warning header as a failure.

Signed-off-by: Tianli Feng <[email protected]>
(cherry picked from commit 91aa312)
tlfeng pushed a commit that referenced this pull request Apr 16, 2022
…emporarily, during deprecation of request parameter 'master_timeout' (#2702)" (#2930) (#2934)

This reverts commit 6a2a33d.
* Sets the High-Level-Rest-Client tests back to treating warning header as a failure.

Signed-off-by: Tianli Feng <[email protected]>
(cherry picked from commit 91aa312)
tlfeng pushed a commit that referenced this pull request Apr 19, 2022
…emporarily, during deprecation of request parameter 'master_timeout' (#2702)" (#2744)

This reverts commit 6a2a33d.

During the process of deprecating REST API request parameter master_timeout and adding alternative parameter cluster_manager_timeout, I made High-Level-Rest-Client tests allow deprecation warning temporarily, by changing the argument of `setStrictDeprecationMode()` to false when building `RestClient` for tests, in the above commit / PR #2702,

This PR sets the High-Level-Rest-Client tests back to treating warning header as a failure.

Signed-off-by: Tianli Feng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport 2.0 Backport to 2.0 branch :test Adding or fixing a test v2.0.0 Version 2.0.0 WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants