OpenSearch icon indicating copy to clipboard operation
OpenSearch copied to clipboard

Feature flags should be defined in opensearch.yml

Open andrross opened this issue 3 years ago • 4 comments

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 avatar Aug 03 '22 00:08 andrross

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:

ntantri avatar Aug 11 '22 20:08 ntantri

@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.

mch2 avatar Aug 24 '22 23:08 mch2

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?

ntantri avatar Aug 25 '22 19:08 ntantri

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

v1bh0r avatar Sep 19 '22 15:09 v1bh0r

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

techrajdeep avatar Oct 09 '22 05:10 techrajdeep

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); https://github.com/opensearch-project/OpenSearch/issues/3818#issuecomment-1261766324

mch2 avatar Oct 11 '22 00:10 mch2

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

mch2 avatar Oct 11 '22 15:10 mch2

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

ntantri avatar Oct 18 '22 12:10 ntantri

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

mch2 avatar Oct 18 '22 16:10 mch2

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

techrajdeep avatar Oct 18 '22 16:10 techrajdeep

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

mch2 avatar Nov 03 '22 00:11 mch2

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

dreamer-89 avatar Dec 27 '22 18:12 dreamer-89

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

ntantri avatar Dec 27 '22 23:12 ntantri

@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 avatar Dec 28 '22 05:12 dreamer-89

Thank you @tan31989 again for working on this issue. As relevant PR https://github.com/opensearch-project/OpenSearch/pull/4959 merged, closing this issue.

dreamer-89 avatar Jan 04 '23 19:01 dreamer-89