Deprecate setting 'cluster.no_master_block' and introduce the alternative setting 'cluster.no_cluster_manager_block' #2453

tlfeng · 2022-03-14T05:52:53Z

Description

Add a new setting cluster.no_cluster_manager_block, aims to replace the existing cluster.no_master_block
Set the value of the old setting cluster.no_master_block as the fallback to the new setting
Deprecate the existing setting cluster.no_master_block
Add unit tests

Reference: commit 63c75d1 / PR #2221

Issues Resolved

Part of #1549

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.

…_cluster_manager_block Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-14T05:54:02Z

Can one of the admins verify this patch?

opensearch-ci-bot · 2022-03-14T06:23:56Z

❌ Gradle Check failure 69c0835
Log 3334

Reports 3334

tlfeng · 2022-03-14T16:39:40Z

In log 3334:

> Task :qa:mixed-cluster:v1.3.0#mixedClusterTest

REPRODUCE WITH: ./gradlew ':qa:mixed-cluster:v1.3.0#mixedClusterTest' --tests "org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT" -Dtests.method="test {p0=indices.get_field_mapping/20_missing_field/Return empty object if field doesn't exist, but index does}" -Dtests.seed=E90871DD815A9AA1 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=da -Dtests.timezone=Pacific/Chuuk -Druntime.java=17

org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT > test {p0=indices.get_field_mapping/20_missing_field/Return empty object if field doesn't exist, but index does} FAILED
    java.lang.AssertionError: Failure at [indices.get_field_mapping/20_missing_field:20]: field [test_index.mappings] is null
        at __randomizedtesting.SeedInfo.seed([E90871DD815A9AA1:615C4E072FA6F759]:0)
        at org.opensearch.test.rest.yaml.OpenSearchClientYamlSuiteTestCase.executeSection(OpenSearchClientYamlSuiteTestCase.java:442)
        at org.opensearch.test.rest.yaml.OpenSearchClientYamlSuiteTestCase.test(OpenSearchClientYamlSuiteTestCase.java:415)

It's reported in issue #2440

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-14T19:33:36Z

✅ Gradle Check success 22907d9
Log 3352

Reports 3352

kartg · 2022-03-15T23:33:00Z

server/src/main/java/org/opensearch/cluster/coordination/NoMasterBlockService.java

+        "cluster.no_cluster_manager_block",
+        NO_MASTER_BLOCK_SETTING,
+        NoMasterBlockService::parseNoMasterBlock,
+        Property.Dynamic,
        Property.NodeScope
    );

    private volatile ClusterBlock noMasterBlock;

    public NoMasterBlockService(Settings settings, ClusterSettings clusterSettings) {


When are we targeting updating the language in places like these?

For the methods that published to maven as Java APIs, I think there are 2 paths.

Replace the name for all of them in a future major version, which will can be tracked in issue Replace "master" terminology in Java APIs #1684.

Create alternative methods and deprecate the existing methods. This is a normal path to deprecate Java APIs. Although I can make the change in separate PRs, there are too many methods with the term "master", it will be a huge effort.
I think a feasible way is to replace the "master" term in them all in a future major version, without adding @deprecate annotation and creating alternative methods. Besides, it's reasonable to choose a few common methods to follow the normal deprecation way to add alternative usages for them.

As @mch2 noted below, I was referring to internal class and method names that include the master language. I agree that incorporating those changes would make the diff rather large. Can you create an issue to track this renaming, if there isn't one already? This can be implemented/committed independent of #1684

Looks like the NoMasterBlockService is published to Maven in jar file (https://opensearch.org/javadocs/1.2.4/OpenSearch/server/build/docs/javadoc/org/opensearch/cluster/coordination/NoMasterBlockService.html), so I thought it's not a internal method. However, seems there are few (or no) software is using APIs in this class.
There are so many classes like this one... I'm not sure if we should follow a normal deprecation path, or just rename.
In addition, there is a issue to track the renaming of internal class and method #1548, but I didn't scan and list all usages in detail.

...c/test/java/org/opensearch/cluster/coordination/NoMasterBlockServiceRenamedSettingTests.java

mch2 · 2022-03-16T00:32:09Z

server/src/internalClusterTest/java/org/opensearch/cluster/NoMasterNodeIT.java

@@ -90,7 +90,7 @@ protected int numberOfReplicas() {
    public void testNoMasterActions() throws Exception {
        Settings settings = Settings.builder()
            .put(AutoCreateIndex.AUTO_CREATE_INDEX_SETTING.getKey(), true)
-            .put(NoMasterBlockService.NO_MASTER_BLOCK_SETTING.getKey(), "all")
+            .put(NoMasterBlockService.NO_CLUSTER_MANAGER_BLOCK_SETTING.getKey(), "all")


Is there something preventing us from updating NoMasterBlockService as well?

Sorry just saw your response to kartg with the same question.

Hi @mch2 thanks for your review! Actually there is nothing blocked. The main reason not to change the class or method name in this PR is to control the size of the code changes in a single PR. Another reason is there are too many class or method that have been published to maven needs renaming, and I didn't target in version 2.0.0 for them.

Missed changes requested by kartg.

…lock Signed-off-by: Tianli Feng <[email protected]>

…alue Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-16T06:07:50Z

❌ Gradle Check failure 301edbc
Log 3434

Reports 3434

tlfeng · 2022-03-16T06:15:24Z

In report 3434:

<html><head></head><body><h1>Class org.opensearch.cluster.remote.test.RemoteClustersIT</h1>
/069927B7DD5D4C71BC076F87491A1FE2962280000CEEA6717B40C/search/qa/remote-clusters/build/reports/tests/integTest/packages/org.opensearch.cluster.remote.test.html">org.opensearch.cluster.remote.test</a> &gt; 

<h3 class="failures">testHAProxyModeConnectionWorks</h3>

java.lang.AssertionError
	at __randomizedtesting.SeedInfo.seed([79FCD9060A4C06DF:7E8A0CCD31CA7082]:0)
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertTrue(Assert.java:53)
	at org.opensearch.cluster.remote.test.RemoteClustersIT.testHAProxyModeConnectionWorks(RemoteClustersIT.java:125)

It's reported in #1703
start gradle check

opensearch-ci-bot · 2022-03-16T06:46:46Z

❌ Gradle Check failure 301edbc
Log 3437

Reports 3437

tlfeng · 2022-03-16T16:33:33Z

In log 3437:

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.action.admin.indices.create.SplitIndexIT.testSplitFromOneToN" -Dtests.seed=90F8DA51F6649AAE -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=nn-NO -Dtests.timezone=Asia/Istanbul -Druntime.java=17

org.opensearch.action.admin.indices.create.SplitIndexIT > testSplitFromOneToN FAILED
    java.lang.AssertionError: ResizeResponse failed - index creation acked but not all shards were started
        at __randomizedtesting.SeedInfo.seed([90F8DA51F6649AAE:5A0D76D37331189C]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked(OpenSearchAssertions.java:140)
        at org.opensearch.action.admin.indices.create.SplitIndexIT.splitToN(SplitIndexIT.java:245)
        at org.opensearch.action.admin.indices.create.SplitIndexIT.testSplitFromOneToN(SplitIndexIT.java:110)

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.action.admin.indices.create.SplitIndexIT.testSplitIndexPrimaryTerm" -Dtests.seed=90F8DA51F6649AAE -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=nn-NO -Dtests.timezone=Asia/Istanbul -Druntime.java=17

org.opensearch.action.admin.indices.create.SplitIndexIT > testSplitIndexPrimaryTerm FAILED
    java.lang.AssertionError: ResizeResponse failed - index creation acked but not all shards were started
        at __randomizedtesting.SeedInfo.seed([90F8DA51F6649AAE:6262B1FF9010364A]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked(OpenSearchAssertions.java:140)
        at org.opensearch.action.admin.indices.create.SplitIndexIT.testSplitIndexPrimaryTerm(SplitIndexIT.java:377)

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.gateway.RecoveryFromGatewayIT.testReuseInFileBasedPeerRecovery" -Dtests.seed=90F8DA51F6649AAE -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=en-IE -Dtests.timezone=Europe/Prague -Druntime.java=17

org.opensearch.gateway.RecoveryFromGatewayIT > testReuseInFileBasedPeerRecovery FAILED
    java.lang.AssertionError: shard [test][0] on node [node_t1] has pending operations:
     --> RetentionLeaseBackgroundSyncAction.Request{retentionLeases=RetentionLeases{primaryTerm=1, version=1569, leases={peer_recovery/Ji_akVeuTe2-peBqVpDhIw=RetentionLease{id='peer_recovery/Ji_akVeuTe2-peBqVpDhIw', retainingSequenceNumber=627, timestamp=1647412450661, source='peer recovery'}, peer_recovery/RYQLVidjQ-qPkBql67DZcQ=RetentionLease{id='peer_recovery/RYQLVidjQ-qPkBql67DZcQ', retainingSequenceNumber=627, timestamp=1647412450661, source='peer recovery'}}}, shardId=[test][0], timeout=1m, index='test', waitForActiveShards=0}
    	at org.opensearch.index.shard.IndexShardOperationPermits.acquire(IndexShardOperationPermits.java:248)
    	at org.opensearch.index.shard.IndexShard.acquirePrimaryOperationPermit(IndexShard.java:3146)

kartg

LGTM assuming gradle check passes eventually 😄

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-17T11:04:41Z

❌ Gradle Check failure 301edbc
Log 3481

Reports 3481

opensearch-ci-bot · 2022-03-17T11:37:12Z

✅ Gradle Check success 36e230a
Log 3483

Reports 3483

Deprecate setting cluster.no_master_block, and add setting cluster.no…

69c0835

…_cluster_manager_block Signed-off-by: Tianli Feng <[email protected]>

tlfeng requested a review from a team as a code owner March 14, 2022 05:52

tlfeng mentioned this pull request Mar 14, 2022

Add alternative usages for the REST APIs and configurations that contain "master" terminology #1549

Closed

12 tasks

tlfeng added enhancement Enhancement or improvement to existing feature or request v2.0.0 Version 2.0.0 documentation pending Tracks issues which have PRs merged but documentation changes pending labels Mar 14, 2022

Corrent setting name in the comment

22907d9

Signed-off-by: Tianli Feng <[email protected]>

dblock requested a review from kartg March 15, 2022 15:44

mch2 requested review from mch2 and removed request for kartg March 15, 2022 19:17

kartg reviewed Mar 15, 2022

View reviewed changes

mch2 previously approved these changes Mar 16, 2022

View reviewed changes

Tianli Feng added 2 commits March 15, 2022 21:44

Merge remote-tracking branch 'upstream/main' into setting-no-master-b…

8470749

…lock Signed-off-by: Tianli Feng <[email protected]>

Flip arguments in assertEquals(), because the first is the expected v…

301edbc

…alue Signed-off-by: Tianli Feng <[email protected]>

kartg approved these changes Mar 16, 2022

View reviewed changes

tlfeng mentioned this pull request Mar 17, 2022

Add a new REST API endpoint 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master' #2404

Merged

5 tasks

Merge branch 'main' into setting-no-master-block

36e230a

Signed-off-by: Tianli Feng <[email protected]>

This was referenced Mar 17, 2022

Add a new node role 'cluster_manager' as the alternative for 'master' role and deprecate 'master' role #2424

Merged

Add 'cluster_manager_node' into ClusterState Metric as an alternative to 'master_node' #2415

Merged

tlfeng merged commit 1193ec1 into opensearch-project:main Mar 18, 2022

tlfeng deleted the setting-no-master-block branch March 18, 2022 21:57

tlfeng mentioned this pull request Mar 22, 2022

Replace internal usages of 'master' term in 'server/src/main' directory #2519

Merged

5 tasks

tlfeng mentioned this pull request Mar 29, 2022

Change the "Blacklist / Whitelist" nomenclature #1483

Closed

10 tasks

tlfeng mentioned this pull request Apr 7, 2022

Add back necessary tests for deprecated settings that are replaced during applying inclusive naming #2805

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate setting 'cluster.no_master_block' and introduce the alternative setting 'cluster.no_cluster_manager_block' #2453

Deprecate setting 'cluster.no_master_block' and introduce the alternative setting 'cluster.no_cluster_manager_block' #2453

tlfeng commented Mar 14, 2022

opensearch-ci-bot commented Mar 14, 2022

opensearch-ci-bot commented Mar 14, 2022

tlfeng commented Mar 14, 2022

opensearch-ci-bot commented Mar 14, 2022

kartg Mar 15, 2022

tlfeng Mar 16, 2022 •

edited

Loading

kartg Mar 16, 2022

tlfeng Mar 17, 2022

mch2 Mar 16, 2022

mch2 Mar 16, 2022

tlfeng Mar 16, 2022

opensearch-ci-bot commented Mar 16, 2022

tlfeng commented Mar 16, 2022 •

edited

Loading

opensearch-ci-bot commented Mar 16, 2022

tlfeng commented Mar 16, 2022

kartg left a comment

opensearch-ci-bot commented Mar 17, 2022

opensearch-ci-bot commented Mar 17, 2022

Deprecate setting 'cluster.no_master_block' and introduce the alternative setting 'cluster.no_cluster_manager_block' #2453

Deprecate setting 'cluster.no_master_block' and introduce the alternative setting 'cluster.no_cluster_manager_block' #2453

Conversation

tlfeng commented Mar 14, 2022

Description

Issues Resolved

Check List

opensearch-ci-bot commented Mar 14, 2022

opensearch-ci-bot commented Mar 14, 2022

tlfeng commented Mar 14, 2022

opensearch-ci-bot commented Mar 14, 2022

kartg Mar 15, 2022

Choose a reason for hiding this comment

tlfeng Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

kartg Mar 16, 2022

Choose a reason for hiding this comment

tlfeng Mar 17, 2022

Choose a reason for hiding this comment

mch2 Mar 16, 2022

Choose a reason for hiding this comment

mch2 Mar 16, 2022

Choose a reason for hiding this comment

tlfeng Mar 16, 2022

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 16, 2022

tlfeng commented Mar 16, 2022 • edited Loading

opensearch-ci-bot commented Mar 16, 2022

tlfeng commented Mar 16, 2022

kartg left a comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 17, 2022

opensearch-ci-bot commented Mar 17, 2022

tlfeng Mar 16, 2022 •

edited

Loading

tlfeng commented Mar 16, 2022 •

edited

Loading