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

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

Merged
merged 20 commits into from
Apr 27, 2022

Conversation

tlfeng
Copy link
Collaborator

@tlfeng tlfeng commented Mar 19, 2022

Description

Replacement rules:

  • master -> cluster_manager or clusterManager (variable name) or cluster-manager (code comment)
  • Master -> ClusterManager

Issues Resolved

A part of #1548

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.

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 11dcbd4
Log 3548

Reports 3548

@tlfeng tlfeng added enhancement Enhancement or improvement to existing feature or request v2.0.0 Version 2.0.0 labels Mar 19, 2022
@tlfeng tlfeng changed the title Replace internal usages of 'master' terminology in server/src/main directory Replace internal usages of 'master' term in server/src/main directory Mar 19, 2022
@tlfeng tlfeng changed the title Replace internal usages of 'master' term in server/src/main directory Replace internal usages of 'master' term in 'server/src/main' directory Mar 19, 2022
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 89ad4b9
Log 3550

Reports 3550

@tlfeng
Copy link
Collaborator Author

tlfeng commented Mar 19, 2022

In log 3550:

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.blocks.SimpleBlocksIT.testAddBlockWhileDeletingIndices" -Dtests.seed=99275B89D9E202D1 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=es-VE -Dtests.timezone=America/El_Salvador -Druntime.java=17

org.opensearch.blocks.SimpleBlocksIT > testAddBlockWhileDeletingIndices FAILED
    com.carrotsearch.randomizedtesting.UncaughtExceptionError: Captured an uncaught exception in thread: Thread[id=977, name=Thread-14, state=RUNNABLE, group=TGRP-SimpleBlocksIT]
        at __randomizedtesting.SeedInfo.seed([99275B89D9E202D1:C851EB9F814D483B]:0)

        Caused by:
        java.lang.AssertionError: 
        Expected: an instance of org.opensearch.index.IndexNotFoundException
             but: <ProcessClusterEventTimeoutException[failed to process cluster event (delete-index [[rlccusjhzk/T9k1DjV3TmavEjk5cD1tiA]]) within 30s]> is a org.opensearch.cluster.metadata.ProcessClusterEventTimeoutException
            at __randomizedtesting.SeedInfo.seed([99275B89D9E202D1]:0)
            at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
            at org.junit.Assert.assertThat(Assert.java:964)
            at org.junit.Assert.assertThat(Assert.java:930)
            at org.opensearch.blocks.SimpleBlocksIT.lambda$testAddBlockWhileDeletingIndices$14(SimpleBlocksIT.java:483)
            at org.opensearch.blocks.SimpleBlocksIT.lambda$testAddBlockWhileDeletingIndices$15(SimpleBlocksIT.java:498)

It's reported in issue #2472
start gradle check

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 89ad4b9
Log 3556

Reports 3556

@tlfeng
Copy link
Collaborator Author

tlfeng commented Mar 19, 2022

In log 3556:

> Task :server:internalClusterTest

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.search.SearchCancellationIT.testCancellationDuringQueryPhaseUsingRequestParameter" -Dtests.seed=FB910982D2688C51 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=it -Dtests.timezone=America/Los_Angeles -Druntime.java=17

org.opensearch.search.SearchCancellationIT > testCancellationDuringQueryPhaseUsingRequestParameter FAILED
    java.lang.AssertionError: At least one shard should have failed. Actual: 0
        at __randomizedtesting.SeedInfo.seed([FB910982D2688C51:7903394CD1A4D942]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failEquals(Assert.java:187)
        at org.junit.Assert.assertNotEquals(Assert.java:201)
        at org.opensearch.search.SearchCancellationIT.ensureSearchWasCancelled(SearchCancellationIT.java:167)
        at org.opensearch.search.SearchCancellationIT.testCancellationDuringQueryPhaseUsingRequestParameter(SearchCancellationIT.java:247)

I couldn't reproduce it locally.

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure eec54ae
Log 3559

Reports 3559

@tlfeng
Copy link
Collaborator Author

tlfeng commented Mar 19, 2022

In log 3559:

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.gateway.RecoveryFromGatewayIT.testReuseInFileBasedPeerRecovery" -Dtests.seed=E5B8689123BC8C1E -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=es-GT -Dtests.timezone=Pacific/Norfolk -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=1498, leases={peer_recovery/VhzdIrX0Taayi_pp9rophg=RetentionLease{id='peer_recovery/VhzdIrX0Taayi_pp9rophg', retainingSequenceNumber=1559, timestamp=1647714373479, source='peer recovery'}, peer_recovery/CqBbYXt5QiSw8OQuzCsgqw=RetentionLease{id='peer_recovery/CqBbYXt5QiSw8OQuzCsgqw', retainingSequenceNumber=1559, timestamp=1647714373479, 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)
    	at org.opensearch.action.support.replication.TransportReplicationAction.acquirePrimaryOperationPermit(TransportReplicationAction.java:1116)
    	at org.opensearch.action.support.replication.TransportReplicationAction$AsyncPrimaryAction.doRun(TransportReplicationAction.java:433)
    	at org.opensearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:50)
    	at org.opensearch.action.support.replication.TransportReplicationAction.handlePrimaryRequest(TransportReplicationAction.java:377)

It's reported in issue #1746.
start gradle check

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure eec54ae
Log 3561

Reports 3561

@tlfeng
Copy link
Collaborator Author

tlfeng commented Mar 19, 2022

In log 3561:

REPRODUCE WITH: ./gradlew ':qa:remote-clusters:integTest' --tests "org.opensearch.cluster.remote.test.RemoteClustersIT.testHAProxyModeConnectionWorks" -Dtests.seed=6C712ACD74C2F3F0 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=lv -Dtests.timezone=America/Guyana -Druntime.java=17

org.opensearch.cluster.remote.test.RemoteClustersIT > testHAProxyModeConnectionWorks FAILED
    java.lang.AssertionError
        at __randomizedtesting.SeedInfo.seed([6C712ACD74C2F3F0:6B07FF064F4485AD]: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 issue #1703.
start gradle check

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure eec54ae
Log 3563

Reports 3563

@tlfeng
Copy link
Collaborator Author

tlfeng commented Mar 19, 2022

In Log 3563:

> Task :qa:rolling-upgrade:v1.3.0#oneThirdUpgradedTest

REPRODUCE WITH: ./gradlew ':qa:rolling-upgrade:v1.3.0#oneThirdUpgradedTest' --tests "org.opensearch.upgrades.IndexingIT.testAutoIdWithOpTypeCreate" -Dtests.seed=512653CB7D65CBC7 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=ja-JP-u-ca-japanese-x-lvariant-JP -Dtests.timezone=Antarctica/Rothera -Druntime.java=17

org.opensearch.upgrades.IndexingIT > testAutoIdWithOpTypeCreate FAILED
    java.lang.AssertionError: there are still running tasks:
    {time_in_queue=389ms, time_in_queue_millis=389, source=shard-started StartedShardEntry{shardId [[recover_and_create_leases_in_promotion][4]], allocationId [gt3B9PnTSYePRTVS4QZdFA], primary term [1], message [after peer recovery]}, executing=true, priority=URGENT, insert_order=301}
    {time_in_queue=388ms, time_in_queue_millis=388, source=shard-started StartedShardEntry{shardId [[recover_and_create_leases_in_promotion][0]], allocationId [yN6_RNTSQViP8oogDc5dQw], primary term [2], message [after peer recovery]}, executing=false, priority=URGENT, insert_order=302}
        at __randomizedtesting.SeedInfo.seed([512653CB7D65CBC7:11BDA0F374A0169E]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.opensearch.test.rest.OpenSearchRestTestCase.lambda$waitForClusterStateUpdatesToFinish$7(OpenSearchRestTestCase.java:783)
        at org.opensearch.test.OpenSearchTestCase.assertBusy(OpenSearchTestCase.java:1060)
        at org.opensearch.test.rest.OpenSearchRestTestCase.waitForClusterStateUpdatesToFinish(OpenSearchRestTestCase.java:774)
        at org.opensearch.test.rest.OpenSearchRestTestCase.cleanUpCluster(OpenSearchRestTestCase.java:358)
...

I couldn't reproduce locally. Created issue 2522 #2522

@tlfeng
Copy link
Collaborator Author

tlfeng commented Mar 19, 2022

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success eec54ae
Log 3566

Reports 3566

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.

Since this PR is "Replace internal usage of master..." do you also want to (shift+F6) refactor ClusterHealthResponse.java#L279 from hasDiscoveredMaster() to hasDiscoveredManager() (or some method name consistent w/ everything else)? Or are you worried that will unknowingly break a plugin that might use it (since the method is public)?

@tlfeng
Copy link
Collaborator Author

tlfeng commented Mar 22, 2022

Since this PR is "Replace internal usage of master..." do you also want to (shift+F6) refactor ClusterHealthResponse.java#L279 from hasDiscoveredMaster() to hasDiscoveredManager() (or some method name consistent w/ everything else)? Or are you worried that will unknowingly break a plugin that might use it (since the method is public)?

Hi @nknize, thanks a lot for reviewing my PR! Yeah, that's my concern, and I wouldn't rename the public methods that published to Maven as Java API in version 2.x, in case the change breaks too many plugins or clients.
There was a discussion in PR #2453 (comment).
Changing such Java APIs will be tracked in issue #1684.
I will rename most of the APIs in at least version 3.0 to give plugins and clients more time, and for some common used APIs, I can follow the normal deprecation path to add "@Deprecation" annotation and create alternative method.

}

private static String getMasterNodeId(ClusterState clusterState) {
private static String getClusterManagerNodeId(ClusterState clusterState) {
if (clusterState == null) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the getMasterNodeId below in line 130 need to be changed to getClusterManagerNodeId?

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 @xuezhou25 , thanks a lot for your time in reviewing my PR! 👍👍.
the method getMasterNodeId() in line 130 is published to Maven as Java API. Please see my comment above: #2519 (comment).
Although the normal way to rename the Java API is to deprecate old method and create new alternative method, there are too many public methods contain name "master", and it will introduce too many extra codes to deprecate them all. So I plan to rename them all in a future major version, and give the other plugins and clients enough notice before renaming.

@@ -201,10 +201,10 @@ protected void doStart(ClusterState clusterState) {
logger.debug("no known master node, scheduling a retry");
Copy link
Contributor

@xuezhou25 xuezhou25 Mar 23, 2022

Choose a reason for hiding this comment

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

line 70
line 185
line 201
line 219
line 282
master->cluster-manager

Copy link
Collaborator Author

@tlfeng tlfeng Mar 23, 2022

Choose a reason for hiding this comment

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

Good catch! Thank you, you are so carefully. Those usages are all missed... 😅
I will change them.

@tlfeng tlfeng requested a review from reta as a code owner April 26, 2022 16:26
Signed-off-by: Tianli Feng <[email protected]>
@tlfeng tlfeng added v2.1.0 Issues and PRs related to version 2.1.0 backport 2.x Backport to 2.x branch and removed WIP Work in progress labels Apr 26, 2022
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure d5660fd
Log 4804

Reports 4804

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure e6c10f2
Log 4808

Reports 4808

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 0c110ca
Log 4818

Reports 4818

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 4d50c6f
Log 4819

Reports 4819

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success cc57d7e
Log 4820

Reports 4820

@dblock dblock merged commit 79eb3b0 into opensearch-project:main Apr 27, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2519-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 79eb3b04922f4d4688372b93550ded9feffd7338
# Push it to GitHub
git push --set-upstream origin backport/backport-2519-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2519-to-2.x.

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 enhancement Enhancement or improvement to existing feature or request v2.1.0 Issues and PRs related to version 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants