Fix #1335: add general-purpose Feature flag system #1390

tatu-at-datastax · 2024-09-04T17:21:57Z

What this PR does:

Adds extensible Feature-Flag system to allow both default, system prop setting, and per-request overrides.

Initially just a single feature is added, "tables":

As YAML Config entry: stargate.feature.flags.tables
As HTTP header: Feature-Flag-tables (case-insensitive)

That is: when starting; may specific fixed enabled/disabled via application.yaml or system property; latter like:

./mvnw quarkus:dev -Dstargate.jsonapi.operations.database-config.local-datacenter=dc1 \
  -Dquarkus.log.console.darken=2  -Dstargate.feature.flags.tables=true

but if no setting specified (and ONLY in that case), per-request curl header like:

curl -i --location --request POST ... --header 'Feature-Flag-tables: true'

can be used to enable/disable processing

Which issue(s) this PR fixes:
Fixes #1355

Checklist

Changes manually tested
Automated Tests added/updated
Documentation added/updated
CLA Signed: DataStax CLA

src/main/resources/application.yaml

…er-call basis

tatu-at-datastax · 2024-09-05T00:33:01Z

Ready for initial review: need to think about how this could be tested better.

…PPER CASE in Enum name)

CONFIGURATION.md

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/NamespaceCache.java

src/main/java/io/stargate/sgv2/jsonapi/api/request/DataApiRequestInfo.java

amorton · 2024-09-05T21:16:50Z

src/main/java/io/stargate/sgv2/jsonapi/api/request/DataApiRequestInfo.java

+    }
+
+    /**
+     * Accessor for getting value of given header (case-insensitive), as {@code Boolean} if (and


nit: does the case insensitivity come from the io.vertx.core.MultiMap ? assuming yes, and that this is because of the HTTP standard, can we make that clear because there is nothing in this code enforcing that

Yes, is due to vertx http server impl that does it for HTTP standard reason.

What I am thinking is that we only document use of lower-case header name for usage, and leave ability for case-insensitivity as impl detail: this since we are not exposing features to actual end users. If case-insensitivity wasn't provided I wouldn't bother actually adding it here.

Hmmh. While I'd prefer all lower-case, looks like general best practice seems to be mixed case.

I am on the edge of my seat here, seems to be ??? seems to be...

hmmmh. Not sure why your GUI showed "seems to be" without second line ("mixed case"). Or is that secret line just shown to me? :-o
(I saw it missing when you screen shared)

src/main/java/io/stargate/sgv2/jsonapi/config/feature/DataApiFeatureConfig.java

amorton · 2024-09-05T21:25:03Z

src/main/java/io/stargate/sgv2/jsonapi/config/feature/DataApiFeatureFlag.java

+
+  private final String featureName;
+
+  private final boolean enabledByDefault;


The command above says "If no configuration specified (config or request), the feature will be Disabled."

consider: we should prob enforce this by default in the enum and remove the enabledByDefault setting. Concern would be that we end up with a mix of values and it gets confusing.

Problem is that precedence is 3-layered:

Explicit value from FeaturesConfig (map keyed by feature)

Explicit value from request header if no (1)

Default if neither (1) or (2).

I originally thought of just leaving (3) to be false for simplicity; I can still do that and drop enabledByDefault as it is not strictly needed -- and can be added if/when needed (or something else that does what is needed).

discussed - drop the enabledByDefault and always default to false. the mental model is these are named boolean values, that default to false.

src/main/java/io/stargate/sgv2/jsonapi/config/feature/DataApiFeatureFlag.java

src/main/java/io/stargate/sgv2/jsonapi/config/feature/DataApiFeatures.java

amorton · 2024-09-05T21:48:19Z

src/main/java/io/stargate/sgv2/jsonapi/config/feature/DataApiFeatures.java

+    // First check if there is definition from configuration
+    Boolean b = fromConfig.get(flag);
+
+    if (b == null) {


if I read this correctly, if the feature is disabled in config it is not possible to enable via request. You mention the three ways for controlling features the other day, may be best to spell this out very clearly at the top fo this class.

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/NamespaceCache.java

amorton · 2024-09-05T21:51:54Z

src/main/resources/application.yaml

@@ -25,6 +25,10 @@ stargate:
  exception-mappers:
    enabled: false

+  feature: # See io.stargate.sgv2.jsonapi.config.feature.DataApiFeatureConfig


not: can we expand the docs here, or point to expanded docs on the DataApiFeatures class to explain the logic for setting values.

Fixed link wrt name, but thinking of documenting only/mostly on code side.

amorton · 2024-09-05T21:55:32Z

src/test/java/io/stargate/sgv2/jsonapi/TestConstants.java

@@ -40,15 +41,21 @@ public final class TestConstants {

  // CommandContext for working on the schema objects above

+  public static final DataApiFeatures DEFAULT_API_FEATURES_FOR_TESTS = DataApiFeatures.empty();


question - how easy / desirable is it to be able to turn on features in a DataApiFeatures class so we can run unit test without needing the things the happen in the DseTestResource class ?

I am thinking that using DseTestResource reflects actual usage better, and as long as we do not have tons of testing for alternative setting (that is, most tests can use specific default), works for now.

Otherwise we may end up adding test scaffolding that differs from actual running.
This wrt ITs.

amorton

generally like where this is going, just comments on names and some details.

tatu-at-datastax · 2024-09-07T00:36:56Z

Ok: situation at the end of week:

Things work wrt Config, Header overrides, with manual testing
Addressed doc/comment suggestions
Figured out how to do ITs, to verify same as (1)
Ran out of time to address one last major thing ("I really want to get the DataApiRequestInfo onto the CommandContext and remove the dependency injection. ") -- hoping to check that on monday.
Aside from (4), ready for re-review!

src/main/java/io/stargate/sgv2/jsonapi/config/feature/ApiFeature.java

vkarpov15

LGTM 👍

amorton · 2024-09-09T18:14:05Z

reviewed in person, Done for me . Will approve, and then we need to get the error object V2 flag to use features config

amorton

approved, see last comment

tatu-at-datastax · 2024-09-09T20:13:26Z

Last changes; will merge once CI passes.

Fix #1355: add general-purpose Feature flag system

975bdc0

tatu-at-datastax commented Sep 4, 2024

View reviewed changes

src/main/resources/application.yaml Outdated Show resolved Hide resolved

tatu-at-datastax added 7 commits September 4, 2024 11:13

Remove Tables-enabled check from NamespaceCache, cannot override on p…

15d73d5

…er-call basis

Fix unit test wrt now passing access to table schema info

2674491

Re-wire enable-tables, temporarily

ba97987

Add sysprop for ITs to enable Tables feature

4cd7526

Incremental progress...

68a3cfb

Wire DataApiFeatures in CommandContext for better access

a4b10e1

Complete initial implementation: things now seem to work as designed

7a5278c

tatu-at-datastax changed the title ~~(WIP) Fix #1355: add general-purpose Feature flag system~~ Fix #1355: add general-purpose Feature flag system Sep 5, 2024

tatu-at-datastax marked this pull request as ready for review September 5, 2024 00:32

tatu-at-datastax requested a review from a team as a code owner September 5, 2024 00:32

tatu-at-datastax added 2 commits September 4, 2024 17:41

Comment fixes

594ebb4

Change API Feature flags to use lower-case names in all config (but U…

4e0fad7

…PPER CASE in Enum name)

tatu-at-datastax commented Sep 5, 2024

View reviewed changes

CONFIGURATION.md Show resolved Hide resolved

tatu-at-datastax commented Sep 5, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/NamespaceCache.java Show resolved Hide resolved

amorton requested changes Sep 5, 2024

View reviewed changes

tatu-at-datastax added 9 commits September 5, 2024 15:25

Renaming from code review

f7920f3

More changes from code review

b68176f

Merge branch 'main' into tatu/1335-feature-flags

c3b6032

...

26ceefa

Fix CreateTableIntegrationTest

fa09a86

Add an IT to verify blocking of CreateTable if feature flag not enabled

f378b9a

Complete IT to verify ApiFeature.TABLES functioning, header override

bb355c7

Final touches wrt naming

e318d5a

Further improve docs (comment)

be9acd7

vkarpov15 reviewed Sep 9, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/config/feature/ApiFeature.java Show resolved Hide resolved

vkarpov15 reviewed Sep 9, 2024

View reviewed changes

amorton approved these changes Sep 9, 2024

View reviewed changes

Last changes from code review

7ca1f3e

tatu-at-datastax merged commit d345c78 into main Sep 9, 2024
3 checks passed

tatu-at-datastax deleted the tatu/1335-feature-flags branch September 9, 2024 20:26

tatu-at-datastax changed the title ~~Fix #1355: add general-purpose Feature flag system~~ Fix #1335: add general-purpose Feature flag system Sep 9, 2024

tatu-at-datastax mentioned this pull request Sep 9, 2024

Refactor feature flags to use combination of Quarkus config and per-request overrides #1335

Closed

vkarpov15 mentioned this pull request Sep 18, 2024

allow setting feature flags stargate/stargate-mongoose#249

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1335: add general-purpose Feature flag system #1390

Fix #1335: add general-purpose Feature flag system #1390

tatu-at-datastax commented Sep 4, 2024 •

edited

Loading

tatu-at-datastax commented Sep 5, 2024

amorton Sep 5, 2024

tatu-at-datastax Sep 5, 2024

tatu-at-datastax Sep 5, 2024

amorton Sep 9, 2024

tatu-at-datastax Sep 10, 2024

amorton Sep 5, 2024

tatu-at-datastax Sep 5, 2024

amorton Sep 9, 2024

amorton Sep 5, 2024

amorton Sep 5, 2024

tatu-at-datastax Sep 7, 2024

amorton Sep 5, 2024

tatu-at-datastax Sep 7, 2024

amorton left a comment

tatu-at-datastax commented Sep 7, 2024

vkarpov15 left a comment

amorton commented Sep 9, 2024

amorton left a comment

tatu-at-datastax commented Sep 9, 2024


		private final String featureName;

		private final boolean enabledByDefault;

		@@ -40,15 +41,21 @@ public final class TestConstants {

		// CommandContext for working on the schema objects above

		public static final DataApiFeatures DEFAULT_API_FEATURES_FOR_TESTS = DataApiFeatures.empty();

Fix #1335: add general-purpose Feature flag system #1390

Fix #1335: add general-purpose Feature flag system #1390

Conversation

tatu-at-datastax commented Sep 4, 2024 • edited Loading

tatu-at-datastax commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amorton left a comment

Choose a reason for hiding this comment

tatu-at-datastax commented Sep 7, 2024

vkarpov15 left a comment

Choose a reason for hiding this comment

amorton commented Sep 9, 2024

amorton left a comment

Choose a reason for hiding this comment

tatu-at-datastax commented Sep 9, 2024

tatu-at-datastax commented Sep 4, 2024 •

edited

Loading