-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Remove] Deprecated Zen1 Discovery #1216
Conversation
start gradle check |
✅ Gradle Wrapper Validation success ec3fc8607114d82a4b7e93e3599a7d051d020c68 |
✅ DCO Check Passed ec3fc8607114d82a4b7e93e3599a7d051d020c68 |
✅ Gradle Precommit success ec3fc8607114d82a4b7e93e3599a7d051d020c68 |
✅ Gradle Check success ec3fc8607114d82a4b7e93e3599a7d051d020c68 |
start gradle check |
✅ DCO Check Passed ed8702c61348985dbf84843b57120b07725b73a8 |
✅ Gradle Wrapper Validation success ed8702c61348985dbf84843b57120b07725b73a8 |
✅ Gradle Precommit success ed8702c61348985dbf84843b57120b07725b73a8 |
❌ Gradle Check failure ed8702c61348985dbf84843b57120b07725b73a8 |
Jenkins flakiness...
Unable to reproduce:
Restarting check... |
start gradle check |
More link checker flakiness... |
✅ Gradle Check success ed8702c61348985dbf84843b57120b07725b73a8 |
Can one of the admins verify this patch? |
✅ Gradle Wrapper Validation success ed8702c61348985dbf84843b57120b07725b73a8 |
✅ DCO Check Passed ed8702c61348985dbf84843b57120b07725b73a8 |
✅ Gradle Precommit success ed8702c61348985dbf84843b57120b07725b73a8 |
✅ Gradle Wrapper Validation success 21ae020f92f3655280be614aef2c920446fe6330 |
✅ Gradle Precommit success 21ae020f92f3655280be614aef2c920446fe6330 |
❌ Gradle Check failure 21ae020f92f3655280be614aef2c920446fe6330 |
Zen1 discovery was deprecated in Legacy 7.x for eventual removal. OpenSearch 1.x carries this deprecation. This commit completely removes all support for Zen1 discovery in favor of Zen2. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
✅ Gradle Wrapper Validation success 47ee204 |
✅ Gradle Precommit success 47ee204 |
@opensearch-project/opensearch-core this is ready for review if anyone wants to have a look. Full zen1 removal which was deprecated in the fork. |
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.
LGTM - Thanks Nick!
Just a couple of comments. I also see a few more references of zen1, but I guess we can clean those up in a subsequent PR.
@@ -670,10 +660,6 @@ void becomeCandidate(String method) { | |||
peerFinder.activate(coordinationState.get().getLastAcceptedState().nodes()); | |||
clusterFormationFailureHelper.start(); | |||
|
|||
if (getCurrentTerm() == ZEN1_BWC_TERM) { |
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.
Should we be completely removing this constant ? It's used here as well
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.
good catch... pushing additional commit to remove all remaining references
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.
Removing caused a node join failure so I'll take care of that in a follow up to isolate the issue.
Failing test:
./gradlew ':server:test' --tests "org.opensearch.node.NodeTests.testCloseOnLeakedIndexReaderReference" -Dtests.seed=47F5C424B97CB691 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=fr-BE -Dtests.timezone=Africa/Maputo -Druntime.java=17
server/src/main/java/org/opensearch/rest/action/admin/indices/RestResizeHandler.java
Outdated
Show resolved
Hide resolved
✅ Gradle Wrapper Validation success d858ffa5f50a3e525bb1d46eda1f385a7f28a832 |
✅ Gradle Wrapper Validation success 602ef840a4948083c353f72b63b1fd6fac3e16c1 |
❌ Gradle Precommit failure d858ffa5f50a3e525bb1d46eda1f385a7f28a832 |
✅ Gradle Precommit success 602ef840a4948083c353f72b63b1fd6fac3e16c1 |
❌ Gradle Check failure d858ffa5f50a3e525bb1d46eda1f385a7f28a832 |
❌ Gradle Check failure 602ef840a4948083c353f72b63b1fd6fac3e16c1 |
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
✅ Gradle Wrapper Validation success 18324ac |
✅ Gradle Precommit success 18324ac |
@@ -293,6 +292,11 @@ | |||
}, Setting.Property.NodeScope); | |||
|
|||
private static final String CLIENT_TYPE = "node"; | |||
public static final Setting<TimeValue> INITIAL_STATE_TIMEOUT_SETTING = Setting.positiveTimeSetting( |
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.
That looks a bit off: why discovery setting on node? May be it is better keep old DiscoverySettings
class with this single setting (for now)?
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.
It's a NodeScope
setting refactored from DiscoverySettings
and the only remaining setting for node discovery so I think it's appropriately refactored here to node level without the overhead of an inner 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.
Sorry, should have been more clear, NodeScope
is totally fine, I think the pretty useful convention we have is that that Node
settings are prefixed with node.
, Search
settings with search.
, Index
settings with index.
, the overhead of inner class is super minimal but it would be just natural to look for discovery.
under Discovery
- related classes, just my two cents.
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.
oic what you're saying. API wise? sure we can make a public inner class. And I think "overhead" should be 1:1? Instead of loading DiscoverySettings.class
(which is now removed) we're adding a new Node$DiscoverySettings.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.
👍 I refactored DiscoverySettings
as an internal class to Node
. I think that's a nice clean suggestion.
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 @nknize !
server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java
Show resolved
Hide resolved
…rnal class Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
✅ Gradle Wrapper Validation success a6f36a2 |
✅ Gradle Precommit success a6f36a2 |
@@ -294,6 +293,14 @@ | |||
|
|||
private static final String CLIENT_TYPE = "node"; | |||
|
|||
public static class DiscoverySettings { |
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.
👍
Zen1 discovery was deprecated in Legacy 7.x for eventual removal. OpenSearch 1.x
carries this deprecation. This PR completely removes all support for Zen1
discovery in favor of Zen2 in the 2.x versioning line.