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

Added support for feature flags in opensearch.yml (#4959) #5707

Closed

Conversation

ntantri
Copy link
Contributor

@ntantri ntantri commented Jan 5, 2023

(cherry picked from commit 241bd42)

Description

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to fetch flag settings defined in opensearch.yml.

Issues Resolved

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
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Gradle Check (Jenkins) Run Completed with:

@ntantri ntantri force-pushed the backport/backport-4959-to-2.x branch from a6d94fe to b296a89 Compare January 5, 2023 08:45
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Gradle Check (Jenkins) Run Completed with:

CHANGELOG.md Outdated
Comment on lines 8 to 42
- Add feature flag for extensions ([#5211](https://github.com/opensearch-project/OpenSearch/pull/5211))
### Added
- Apply reproducible builds configuration for OpenSearch plugins through gradle plugin ([#4746](https://github.com/opensearch-project/OpenSearch/pull/4746))
- Prevent deletion of snapshots that are backing searchable snapshot indexes ([#5069](https://github.com/opensearch-project/OpenSearch/pull/5069))
- Update to Gradle 7.6 ([#5382](https://github.com/opensearch-project/OpenSearch/pull/5382))
- Reject bulk requests with invalid actions ([#5299](https://github.com/opensearch-project/OpenSearch/issues/5299))
- Add max_shard_size parameter for shrink API ([#5229](https://github.com/opensearch-project/OpenSearch/pull/5229))
- Added jackson dependency to server ([#5366] (https://github.com/opensearch-project/OpenSearch/pull/5366))
- Adding support to register settings dynamically ([#5495](https://github.com/opensearch-project/OpenSearch/pull/5495))
- Adding auto release workflow ([#5582](https://github.com/opensearch-project/OpenSearch/pull/5582))
- Added experimental support for extensions ([#5347](https://github.com/opensearch-project/OpenSearch/pull/5347)), ([#5518](https://github.com/opensearch-project/OpenSearch/pull/5518)), ([#5597](https://github.com/opensearch-project/OpenSearch/pull/5597)), ([#5615](https://github.com/opensearch-project/OpenSearch/pull/5615)))
- Add CI bundle pattern to distribution download ([#5348](https://github.com/opensearch-project/OpenSearch/pull/5348))
- Added @gbbafna as an OpenSearch maintainer ([#5668](https://github.com/opensearch-project/OpenSearch/pull/5668))
- Add feature flag for extensions ([#5211](https://github.com/opensearch-project/OpenSearch/pull/5211))
- Add support for s390x architecture ([#4001](https://github.com/opensearch-project/OpenSearch/pull/4001))
- Github workflow for changelog verification ([#4085](https://github.com/opensearch-project/OpenSearch/pull/4085))
- Point in time rest layer changes for create and delete PIT API ([#4064](https://github.com/opensearch-project/OpenSearch/pull/4064))
- Add failover support with Segment Replication enabled. ([#4325](https://github.com/opensearch-project/OpenSearch/pull/4325)
- Point in time rest layer changes for list PIT and PIT segments API ([#4388](https://github.com/opensearch-project/OpenSearch/pull/4388))
- Added @dreamer-89 as an Opensearch maintainer ([#4342](https://github.com/opensearch-project/OpenSearch/pull/4342))
- Added release notes for 1.3.5 ([#4343](https://github.com/opensearch-project/OpenSearch/pull/4343))
- Added release notes for 2.2.1 ([#4344](https://github.com/opensearch-project/OpenSearch/pull/4344))
- Label configuration for dependabot PRs ([#4348](https://github.com/opensearch-project/OpenSearch/pull/4348))
- Support for HTTP/2 (server-side) ([#3847](https://github.com/opensearch-project/OpenSearch/pull/3847))
- BWC version 2.2.2 ([#4383](https://github.com/opensearch-project/OpenSearch/pull/4383))
- Support for labels on version bump PRs, skip label support for changelog verifier ([#4391](https://github.com/opensearch-project/OpenSearch/pull/4391))
- Add a new node role 'search' which is dedicated to provide search capability ([#4689](https://github.com/opensearch-project/OpenSearch/pull/4689))
- Introduce experimental searchable snapshot API ([#4680](https://github.com/opensearch-project/OpenSearch/pull/4680))
- Added support for feature flags in opensearch.yml ([#4959](https://github.com/opensearch-project/OpenSearch/pull/4959))
Copy link
Member

Choose a reason for hiding this comment

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

@tan31989 : Looks like lot many incorrect (entries from main?) are also been backported. Can you please fix this ?

Copy link
Contributor Author

@ntantri ntantri Jan 7, 2023

Choose a reason for hiding this comment

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

@dreamer-89 I verified these manually; only two didn't have the file changes in the 2.x. I have removed those 2 from the CHANGELOG.md. The rest of these have files in the 2.x branch but are never mentioned in the CHANGELOG.md.

I have just "cherry-picked" by committing via command for backport.

Does this mean, I need to keep it here or remove those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just FYI, now trying to fix the merge conflicts and failing tests once I had to rebase this branch to 2.x, hence there are multiple pushes to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @tan31989 for the effort on this PR.

@dreamer-89 I verified these manually; only two didn't have the file changes in the 2.x. I have removed those 2 from the CHANGELOG.md. The rest of these have files in the 2.x branch but are never mentioned in the CHANGELOG.md.

I see. Thanks for fixing the missing changelog entries.

@ntantri ntantri force-pushed the backport/backport-4959-to-2.x branch from b296a89 to 51e344e Compare January 7, 2023 13:37
@ntantri ntantri requested a review from VachaShah as a code owner January 7, 2023 13:37
@ntantri ntantri force-pushed the backport/backport-4959-to-2.x branch from 51e344e to 523cd72 Compare January 7, 2023 13:44
@ntantri ntantri force-pushed the backport/backport-4959-to-2.x branch from 523cd72 to 2e93b82 Compare January 8, 2023 04:28
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2023

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

Codecov Report

Merging #5707 (2e93b82) into 2.x (ee33c9e) will decrease coverage by 0.51%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##                2.x    #5707      +/-   ##
============================================
- Coverage     71.02%   70.50%   -0.52%     
+ Complexity    58984    58559     -425     
============================================
  Files          4741     4742       +1     
  Lines        280607   280629      +22     
  Branches      40894    40897       +3     
============================================
- Hits         199295   197871    -1424     
- Misses        65099    66290    +1191     
- Partials      16213    16468     +255     
Impacted Files Coverage Δ
...a/org/opensearch/test/OpenSearchIntegTestCase.java 54.51% <0.00%> (-2.11%) ⬇️
...pensearch/common/settings/FeatureFlagSettings.java 50.00% <50.00%> (ø)
.../java/org/opensearch/common/util/FeatureFlags.java 80.00% <88.88%> (+30.00%) ⬆️
server/src/main/java/org/opensearch/Version.java 83.40% <100.00%> (+0.07%) ⬆️
...org/opensearch/common/settings/SettingsModule.java 84.45% <100.00%> (+0.32%) ⬆️
server/src/main/java/org/opensearch/node/Node.java 83.33% <100.00%> (+0.02%) ⬆️
...n/indices/forcemerge/ForceMergeRequestBuilder.java 0.00% <0.00%> (-75.00%) ⬇️
.../indices/forcemerge/TransportForceMergeAction.java 25.00% <0.00%> (-75.00%) ⬇️
...port/ResponseHandlerFailureTransportException.java 0.00% <0.00%> (-60.00%) ⬇️
...a/org/opensearch/client/cluster/SniffModeInfo.java 0.00% <0.00%> (-56.25%) ⬇️
... and 486 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ntantri ntantri force-pushed the backport/backport-4959-to-2.x branch 3 times, most recently from d4e80b8 to fbf43db Compare January 8, 2023 11:40
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2023

Gradle Check (Jenkins) Run Completed with:

@ntantri ntantri force-pushed the backport/backport-4959-to-2.x branch from fbf43db to cd4bab2 Compare January 8, 2023 12:08
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2023

Gradle Check (Jenkins) Run Completed with:

@ntantri ntantri force-pushed the backport/backport-4959-to-2.x branch from cd4bab2 to 7ddf790 Compare January 8, 2023 12:31
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2023

Gradle Check (Jenkins) Run Completed with:

@ntantri ntantri force-pushed the backport/backport-4959-to-2.x branch from 7ddf790 to b3ee17d Compare January 8, 2023 13:02
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2023

Gradle Check (Jenkins) Run Completed with:

@ntantri ntantri force-pushed the backport/backport-4959-to-2.x branch from b3ee17d to b5084ee Compare January 8, 2023 13:30
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      2 org.opensearch.snapshots.SegmentReplicationSnapshotIT.testSnapshotOnSegRep_RestoreOnSegRepDuringIngestion

@dreamer-89
Copy link
Member

dreamer-89 commented Jan 9, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      2 org.opensearch.snapshots.SegmentReplicationSnapshotIT.testSnapshotOnSegRep_RestoreOnSegRepDuringIngestion

Another segment replication flaky test. There are multiple test failures which is worrysome. @tan31989 Let's wait on this PR until we identify cause of this flakyness. We are priortizing fixing these tests as part of #5669. Please feel free to pick some if you are interested.

@ntantri
Copy link
Contributor Author

ntantri commented Jan 10, 2023

Another segment replication flaky test. There are multiple test failures which is worrysome. @tan31989 Let's wait on this PR until we identify cause of this flakyness. We are priortizing fixing these tests as part of #5669. Please feel free to pick some if you are interested.

Yes, that makes sense. I believe if there are any fixes to follow up on, I will keep an eye open for merge conflicts if any.

@dreamer-89
Copy link
Member

Another segment replication flaky test. There are multiple test failures which is worrysome. @tan31989 Let's wait on this PR until we identify cause of this flakyness. We are priortizing fixing these tests as part of #5669. Please feel free to pick some if you are interested.

Yes, that makes sense. I believe if there are any fixes to follow up on, I will keep an eye open for merge conflicts if any.

Hi @tan31989, thank you for helping on this issue. We analyzed the flaky tests, most of them are failing due to doc count mismatch. There is a PR on the way to fix this. I think we can go ahead and merge this PR. Can you please resolve the merge conflicts and rebase your changes.

…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <nagarajtantri@gmail.com>

(cherry picked from commit 241bd42)
@ntantri ntantri force-pushed the backport/backport-4959-to-2.x branch from b5084ee to 272edee Compare January 19, 2023 11:27
@ntantri ntantri requested a review from gbbafna as a code owner January 19, 2023 11:27
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dreamer-89
Copy link
Member

Gradle Check (Jenkins) Run Completed with:

Segrep test failure testSnapshotOnSegRep_RestoreOnSegRepDuringIngestion. This is already tracked in #5669. Refiring the gradle check!

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.snapshots.SegmentReplicationSnapshotIT.testSnapshotOnSegRep_RestoreOnSegRepDuringIngestion" -Dtests.seed=692E412BA80E078D -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=es-NI -Dtests.timezone=America/Kentucky/Monticello -Druntime.java=17

org.opensearch.snapshots.SegmentReplicationSnapshotIT > testSnapshotOnSegRep_RestoreOnSegRepDuringIngestion FAILED
    java.lang.AssertionError: timed out waiting for green state
        at __randomizedtesting.SeedInfo.seed([692E412BA80E078D:46A1E74C1F5EF990]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.opensearch.test.OpenSearchIntegTestCase.ensureColor(OpenSearchIntegTestCase.java:1007)
        at org.opensearch.test.OpenSearchIntegTestCase.ensureGreen(OpenSearchIntegTestCase.java:938)
        at org.opensearch.test.OpenSearchIntegTestCase.ensureGreen(OpenSearchIntegTestCase.java:927)
        at org.opensearch.snapshots.SegmentReplicationSnapshotIT.testSnapshotOnSegRep_RestoreOnSegRepDuringIngestion(SegmentReplicationSnapshotIT.java:180)

- Support for labels on version bump PRs, skip label support for changelog verifier ([#4391](https://github.com/opensearch-project/OpenSearch/pull/4391))
- Add a new node role 'search' which is dedicated to provide search capability ([#4689](https://github.com/opensearch-project/OpenSearch/pull/4689))
- Introduce experimental searchable snapshot API ([#4680](https://github.com/opensearch-project/OpenSearch/pull/4680))
- Added support for feature flags in opensearch.yml ([#4959](https://github.com/opensearch-project/OpenSearch/pull/4959))
Copy link
Member

Choose a reason for hiding this comment

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

Please add only your change within the changelog.

Suggested change
- Added support for feature flags in opensearch.yml ([#4959](https://github.com/opensearch-project/OpenSearch/pull/4959))
- Added support for feature flags in opensearch.yml ([#4959](https://github.com/opensearch-project/OpenSearch/pull/4959))

Copy link
Member

Choose a reason for hiding this comment

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

FYI @mch2 and @dreamer-89, the reason cherry-pick got so confused by this changelog entry is that it was added to the wrong section on the main branch. Since the changed was destined for a 2.x release, the changelog entry should have gone in the [Unreleased 2.x] section.

@andrross
Copy link
Member

Thanks @tan31989! This backport is pretty messy for a couple different reasons, so I went ahead and created another attempt here: #5941

@kotwanikunal
Copy link
Member

#5941 has been merged in to 2.x. Thanks for your contribution @tan31989!
I am closing this PR. Please re-open if there is any change pending.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants