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

Feature flags should be defined in opensearch.yml #4102

Closed
Tracked by #5147
andrross opened this issue Aug 3, 2022 · 15 comments
Closed
Tracked by #5147

Feature flags should be defined in opensearch.yml #4102

andrross opened this issue Aug 3, 2022 · 15 comments
Assignees
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request good first issue Good for newcomers hacktoberfest Global event that encourages people to contribute to open-source. low hanging fruit

Comments

@andrross
Copy link
Member

andrross commented Aug 3, 2022

Currently, these flags are passed as JVM args to the process that launches the server. The general (though not universal) convention today is that JVM args are mostly used for tuning the JVM itself, and OpenSearch application configuration is specified via settings in opensearch.yml. It would make for a better user experience for the feature flags to be a configuration option in opensearch.yml, i.e. something like experimental.features: ["feature-1", "feature-2"].

See the previous discussion in #4024

@andrross andrross added enhancement Enhancement or improvement to existing feature or request good first issue Good for newcomers low hanging fruit distributed framework labels Aug 3, 2022
@ntantri
Copy link
Contributor

ntantri commented Aug 11, 2022

Hi,

I am giving my thoughts on this issue. Ideally, feature flags should be allowed to toggle, but with YAML that won't be easier. The ideal technique to allow toggling of features is via an API or Database. So, was wondering if this is still a valid use case because I felt it could be termed as configuration management and not a feature flag.

Whether the convention sticks along or not, I would like to give this PR a try.

Do let me know your thoughts and also if possible please answer these doubts for me:

@mch2
Copy link
Member

mch2 commented Aug 24, 2022

@tan31989 Thanks for your interest in this issue.

Hi,

I am giving my thoughts on this issue. Ideally, feature flags should be allowed to toggle, but with YAML that won't be easier. The ideal technique to allow toggling of features is via an API or Database. So, was wondering if this is still a valid use case because I felt it could be termed as configuration management and not a feature flag.

The idea behind these feature flags is that they gate an experimental feature that is not ready to be enabled in production. With the flag enabled, an additional setting becomes available that may toggle a particular feature on/off. For example for Segment Replication, the feature flag currently needs to be enabled via a jvm flag, in addition to the index created with the setting enabled.

curl -X PUT "localhost:9200/<index-name>?pretty" -H 'Content-Type: application/json' -d '{
  "settings": {
    "index": {
      "replication": {
        "type" : "SEGMENT"
      }
    }
  }
}'

Without the flag enabled, attempting the request above will return that the setting is not found.

{
  "error" : {
    "root_cause" : [
      {
        "type" : "illegal_argument_exception",
        "reason" : "unknown setting [index.replication.type] please check that any required plugins are installed, or check the breaking changes documentation for removed settings"
      }
    ],
    "type" : "illegal_argument_exception",
    "reason" : "unknown setting [index.replication.type] please check that any required plugins are installed, or check the breaking changes documentation for removed settings"
  },
  "status" : 400
}

The flags could be defined in opensearch.yml manually or provided as default options here to be included in the distribution.

  • Currently, the master branch has these two flags. So I can consider adding those as false by default?

Correct these are currently the only two flags.

@ntantri
Copy link
Contributor

ntantri commented Aug 25, 2022

Thanks, @mch2 for the details.

How can one test if it's added as part of the default options here of the distribution?

Could you provide some more inputs on the build and testing?

@v1bh0r
Copy link

v1bh0r commented Sep 19, 2022

Hi @tan31989, check this out - https://discuss.elastic.co/t/how-to-setup-unit-testing-for-elastissearch-configuration/11016/2
Seems like the configurations can be set using System.setProperty

@Poojita-Raj Poojita-Raj added the hacktoberfest Global event that encourages people to contribute to open-source. label Sep 29, 2022
@techrajdeep
Copy link

I am looking for an issue for my first contribution . Can you help me here. Can I take on this .

@mch2
Copy link
Member

mch2 commented Oct 11, 2022

Sorry for the no reply here @tan31989.

To test I would suggest creating a test feature flag that is passed in an IT as a node level setting with internalCluster().startNode(nodeSettings);. You can then use the settings API to fetch node level settings and see that the flag is enabled.

I would also test with an IT that uses existing flags, you can also enable the flag at the node scope and run the test as normal. Example for Segment Replication.

@xuezhou25 is looking to make the flags pluggable for our integ tests so they can be injected into the Node/Environment rather than hardcoded in jvm args. I don't think #3818 makes sense if we are moving to a config setting, because we can set the node setting with internalCluster().startNode(nodeSettings);
#3818 (comment)

@mch2
Copy link
Member

mch2 commented Oct 11, 2022

@tan31989 Do you still have interest in picking this up? If not @techrajdeep its all yours.

@ntantri
Copy link
Contributor

ntantri commented Oct 18, 2022

@mch2 will give it a try. I was waiting for an answer and was not available for a week. Thanks for giving the inputs.

@mch2
Copy link
Member

mch2 commented Oct 18, 2022

Thanks @tan31989 I will assign to you. Let me know if you have further questions on the issue.

@techrajdeep
Copy link

@mch2 If there any other issue for first time to start with , you can assign me

@mch2
Copy link
Member

mch2 commented Nov 3, 2022

Thanks for your interest @techrajdeep! I would suggest filtering by the tag good first issue to find these issues.

@dreamer-89
Copy link
Member

@tan31989: Are you still working on this issue ? Please let know in case you need any help.

@ntantri
Copy link
Contributor

ntantri commented Dec 27, 2022

@dreamer-89, apologies for the delay. Yes, I am still trying to work on this. I should have this completed by the 31st.

@dreamer-89
Copy link
Member

@dreamer-89, apologies for the delay. Yes, I am still trying to work on this. I should have this completed by the 31st.

Thank you @tan31989 for working on this issue and updating on progress. Feel free to post here in case you are stuck.

@dreamer-89
Copy link
Member

Thank you @tan31989 again for working on this issue. As relevant PR #4959 merged, closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request good first issue Good for newcomers hacktoberfest Global event that encourages people to contribute to open-source. low hanging fruit
Projects
None yet
Development

No branches or pull requests

7 participants