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

Add Feature Toggles #18699

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

branimir-vujicic
Copy link
Contributor

@branimir-vujicic branimir-vujicic commented Nov 17, 2022

Feature Toggles should allow teams to modify system behavior without changing code. Feature Toggles are configured using google guice. Basic definition of toggles are crated using FeatureToggleBinder. FeatureToggleBinder creates FeatureToggle and additional configuration can be done using feature configuration.
In current stage Feature Toggles supports:

  • if / else based feature toggles
  • Dependency Injection based Hot reloading implementation without restart require code refactoring to add an interface when injecting the new implementation/class
  • using various toggle strategies along with simple on / off toggles

configuration:

to allow feature toggle configuration four lines are needed in config.properties file

features.config-source-type=file
features.refresh-period=30s

configuration-source-type is source type for Feature Toggles configuration features.config-source is a source (file) of the configuration features.config-type format in which configuration is stored (json or properties) features.refresh-period configuration refresh period

Defining Feature Toggles

Feature toggle definition is done in google guice module using FeatureToggleBinder

simple feature toggle definition

    featureToggleBinder(binder)
                        .featureId("featureXX")
                        .bind()

This example creates bindings for @Inject

    @Inject
    public Runner(@FeatureToggle("featureXX") Supplier<Boolean> isFeatureXXEnabled)
    {
        this.isFeatureXXEnabled = isFeatureXXEnabled;
    }

isFeatureXXEnabled can be used to test if feature is enabled or disabled:

    boolean testFeatureXXEnabled()
    {
        return isFeatureXXEnabled.get();
    }

hot reloadable feature toggle definition

featureToggleBinder(binder, Feature01.class)
                        .featureId("feature01")
                        .baseClass(Feature01.class)
                        .defaultClass(Feature01Impl01.class)
                        .allOf(Feature01Impl01.class, Feature01Impl02.class)
                        .bind()

adding Feature Toggle switching strategy

featureToggleBinder(binder)
                        .featureId("feature04")
                        .toggleStrategy("AllowAll")
                        .toggleStrategyConfig(ImmutableMap.of("key", "value", "key2", "value2"))

feature-config.properties file example

# feature query-logger
feature.query-logger.enabled=true
feature.query-logger.strategy=OsToggle
feature.query-logger.strategy.os_name=.*Linux.*

#feature.query-rate-limiter
feature.query-rate-limiter.currentInstance=com.facebook.presto.server.protocol.QueryBlockingRateLimiter

# feature.query-cancel
feature.query-cancel.strategy=AllowList
feature.query-cancel.strategy.allow-list-source=.*IDEA.*
feature.query-cancel.strategy.allow-list-user=.*prestodb

in this example for first feature query-logger
changing value of feature.query-logger.enabled to false will 'disable' this feature. Changes will be effective within refresh period.

Test plan - Unit Tests

== RELEASE NOTES ==

General Changes
 * Add support for Feature Toggles.

@v-jizhang
Copy link
Contributor

@bot kick off tests

@branimir-vujicic branimir-vujicic force-pushed the features/feature_toggle_pr branch 2 times, most recently from 19b43a4 to 420f154 Compare November 18, 2022 11:55
Copy link
Contributor

@ericyuliu ericyuliu left a comment

Choose a reason for hiding this comment

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

Shall we add more documentation/comments inside code, for each new component, annotation use case, and each test case as a living doc.

@branimir-vujicic branimir-vujicic force-pushed the features/feature_toggle_pr branch 3 times, most recently from 235b92d to c388407 Compare December 15, 2022 13:10
@branimir-vujicic branimir-vujicic force-pushed the features/feature_toggle_pr branch 4 times, most recently from 9778a00 to b9c6a41 Compare January 18, 2023 15:28
@branimir-vujicic branimir-vujicic force-pushed the features/feature_toggle_pr branch 3 times, most recently from 313bdf0 to eaf007a Compare January 30, 2023 18:51
@branimir-vujicic branimir-vujicic marked this pull request as ready for review January 30, 2023 19:06
@branimir-vujicic branimir-vujicic requested a review from a team as a code owner January 30, 2023 19:06
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

Can you remove the custom parsing code and reuse the code in airlift/configuration to parse individual FeatureConfigurations?

@branimir-vujicic branimir-vujicic force-pushed the features/feature_toggle_pr branch 2 times, most recently from 063f700 to b15bd98 Compare March 16, 2023 14:16
@branimir-vujicic branimir-vujicic force-pushed the features/feature_toggle_pr branch 2 times, most recently from 27bcf68 to 21fc7fe Compare August 16, 2023 14:54
@branimir-vujicic branimir-vujicic force-pushed the features/feature_toggle_pr branch 2 times, most recently from d9da818 to 754839f Compare August 17, 2023 09:20
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

One major thing that seems to be missing is: I thought there would be a plugin mechanism that would allow one to change how the toggles are persisted and loaded? Without that, how does one even use the FileBasedFeatureToggleModule, since it seems to be unused in this PR?

import static com.google.common.base.Suppliers.memoizeWithExpiration;
import static java.util.concurrent.TimeUnit.MILLISECONDS;

public class FileBasedFeatureToggleModule
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FileBasedFeatureToggleModule is a FeatureToggle module for file-based configuration.
For other configuration sources, we can create and initialize other modules. for example: DatabaseBasedFeatureToggleModule.

I'll try to refactor this and extract the configuration sources parser in a separate class, and we will have a FeatureToggleModule and configuration parsers based on configuration sources

@branimir-vujicic
Copy link
Contributor Author

HI @tdcmeehan,
I updated the configuration loading to use the plugin mechanism.

@tdcmeehan
Copy link
Contributor

Thank you for refactoring this into a plugin. Can you please make sure that all the file-based loading code is in one module, and all the core code to wire in feature toggles is in a separate module? One may want to swap out the file reading code with something e.g. db backed, without depending on the file loading code.

What happens if we add a feature toggle in the code, but the operator hasn't added a configuration source plugin?

@branimir-vujicic
Copy link
Contributor Author

HI @tdcmeehan,
I separated the feature toggle module and the feature toggle plugin. Now, the file configuration is in presto-feature-toggle-plugin.
If no configuration source is configured feature toggles work with a dummy configuration source.

@ajaygeorge
Copy link
Contributor

@branimir-vujicic This requires a release note since this is introducing a new feature. Can you please add that in.

@ajaygeorge
Copy link
Contributor

@branimir-vujicic Are the Velox changes needed as a part of this PR. ? Seems unrelated. Can you please remove them.

@branimir-vujicic
Copy link
Contributor Author

@ajaygeorge Velox updates are included by mistake.
updated release notes.

@branimir-vujicic branimir-vujicic force-pushed the features/feature_toggle_pr branch 2 times, most recently from a5d9d0a to 951249c Compare September 7, 2023 07:24
Feature Toggles should allow teams to modify system behavior without changing
code. Feature Toggles are configured using google guice. Basic definition of
toggles are crated using FeatureToggleBinder. FeatureToggleBinder creates
FeatureToggle and additional configuration can be done using feature
configuration.
In current stage Feature Toggles supports:
- if / else based feature toggles
- Dependency Injection based
  Hot reloading implementation without restart require code refactoring to
  add an interface when injecting the new implementation/class
- using various toggle strategies along with simple on / off toggles

configuration:

to allow feature toggle configuration four lines are needed in
config.properties file
```
features.config-source-type=file
features.config-source=/etc/feature-config.properties
features.config-type=properties
features.refresh-period=30s
```

`configuration-source-type` is source type for Feature Toggles configuration
`features.config-source` is a source (file) of the configuration
`features.config-type` format in which configuration is stored (json or properties)
`features.refresh-period` configuration refresh period

Defining Feature Toggles

Feature toggle definition is done in google guice module using `FeatureToggleBinder`

simple feature toggle definition
```
    featureToggleBinder(binder)
                        .featureId("featureXX")
                        .bind()
```
This example creates bindings for @Inject
```
    @Inject
    public Runner(@FeatureToggle("featureXX") Supplier<Boolean> isFeatureXXEnabled)
    {
        this.isFeatureXXEnabled = isFeatureXXEnabled;
    }
```
`isFeatureXXEnabled` can be used to test if feature is enabled or disabled:
```
    boolean testFeatureXXEnabled()
    {
        return isFeatureXXEnabled.get();
    }
```

hot reloadable feature toggle definition
```
featureToggleBinder(binder, Feature01.class)
                        .featureId("feature01")
                        .baseClass(Feature01.class)
                        .defaultClass(Feature01Impl01.class)
                        .allOf(Feature01Impl01.class, Feature01Impl02.class)
                        .bind()
```
adding Feature Toggle switching strategy
```
featureToggleBinder(binder)
                        .featureId("feature04")
                        .toggleStrategy("AllowAll")
                        .toggleStrategyConfig(ImmutableMap.of("key", "value", "key2", "value2"))
```

feature-config.properties file example
```
# feature query-logger
feature.query-logger.enabled=true
feature.query-logger.strategy=OsToggle
feature.query-logger.strategy.os_name=.*Linux.*

#feature.query-rate-limiter
feature.query-rate-limiter.currentInstance=com.facebook.presto.server.protocol.QueryBlockingRateLimiter

# feature.query-cancel
feature.query-cancel.strategy=AllowList
feature.query-cancel.strategy.allow-list-source=.*IDEA.*
feature.query-cancel.strategy.allow-list-user=.*prestodb
```
in this example for first feature `query-logger`
changing value of feature.query-logger.enabled to `false` will 'disable' this feature.
Changes will be effective within refresh period.
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.

None yet

5 participants