Revert "Add integ test for simulating node join left event when data …#20674
Conversation
📝 WalkthroughWalkthroughThis PR removes test infrastructure and methods from the cluster coordination test suite, including custom listener plugins, lag-simulation test scenarios, and associated test utility methods. The changes affect the test scaffolding in NodeJoinLeftIT, the RequiresTestIndexStoreListener annotation, and public API methods in TestLogsAppender. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/src/internalClusterTest/java/org/opensearch/cluster/coordination/NodeJoinLeftIT.java (1)
75-76: Non-standard javadoc formatting.The javadoc comment is missing the conventional
*prefix on continuation lines. While syntactically valid, standard javadoc format uses asterisks for consistency and readability.Suggested fix
/** - Check https://github.com/opensearch-project/OpenSearch/issues/4874 and - https://github.com/opensearch-project/OpenSearch/pull/15521 for context + * Check https://github.com/opensearch-project/OpenSearch/issues/4874 and + * https://github.com/opensearch-project/OpenSearch/pull/15521 for context */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/internalClusterTest/java/org/opensearch/cluster/coordination/NodeJoinLeftIT.java` around lines 75 - 76, The class-level Javadoc for NodeJoinLeftIT uses continuation lines without the conventional " * " prefix; update the Javadoc above the NodeJoinLeftIT class (or the affected Javadoc block) so each continuation line begins with " * " (i.e., standard Javadoc block form /**\n * First line\n * Check https://... and\n * https://... for context\n */) to match project style and improve readability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/src/internalClusterTest/java/org/opensearch/cluster/coordination/NodeJoinLeftIT.javaserver/src/internalClusterTest/java/org/opensearch/cluster/coordination/RequiresTestIndexStoreListener.javatest/framework/src/main/java/org/opensearch/test/TestLogsAppender.java
💤 Files with no reviewable changes (2)
- server/src/internalClusterTest/java/org/opensearch/cluster/coordination/RequiresTestIndexStoreListener.java
- test/framework/src/main/java/org/opensearch/test/TestLogsAppender.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: Analyze (java)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Mend Security Check
🔇 Additional comments (4)
server/src/internalClusterTest/java/org/opensearch/cluster/coordination/NodeJoinLeftIT.java (4)
88-95: LGTM!The removal of
TestIndexStoreListenerPluginas part of this revert is appropriate. The remaining plugins are standard test infrastructure for transport mocking and index store testing.
109-111: LGTM!The simplified static initialization using
Arrays.asListis appropriate here. The list is only used to configure the log appender at initialization.
119-126: LGTM!Converting
nodeSettingsto a local final variable is appropriate as it's only used withinsetUp(). This reduces class-level state.Note: Lines 121 and 125 both configure
CLUSTER_NODE_RECONNECT_INTERVAL_SETTING(the second value100msoverrides10s). This appears to be pre-existing code, so it's outside the scope of this revert.
127-128: LGTM!The change from
startClusterManagerOnlyNodetostartNoderestores the original test configuration as part of this revert. The first node will now have both cluster-manager and data node roles, which aligns with the simplified test setup after removing the flaky test infrastructure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@server/src/internalClusterTest/java/org/opensearch/cluster/coordination/NodeJoinLeftIT.java`:
- Around line 75-76: The class-level Javadoc for NodeJoinLeftIT uses
continuation lines without the conventional " * " prefix; update the Javadoc
above the NodeJoinLeftIT class (or the affected Javadoc block) so each
continuation line begins with " * " (i.e., standard Javadoc block form /**\n *
First line\n * Check https://... and\n * https://... for context\n */) to match
project style and improve readability.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20674 +/- ##
============================================
+ Coverage 73.19% 73.27% +0.07%
- Complexity 71924 72025 +101
============================================
Files 5781 5781
Lines 329292 329396 +104
Branches 47514 47525 +11
============================================
+ Hits 241026 241365 +339
+ Misses 68925 68624 -301
- Partials 19341 19407 +66 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
…node..."
This reverts the following commits:
These tests have been very flaky since they were introduced in PR #19907
@SwethaGuptha @shwetathareja It would be great to fix these, but we have been unsuccessful in the past couple months (see #20382). Unless we have a quick fix then I think a revert is better in the short term.
Check List
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.