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

Improvements to feature flagging and handling of breaking changes #4446

Open
nwoolmer opened this issue Apr 26, 2024 · 3 comments
Open

Improvements to feature flagging and handling of breaking changes #4446

nwoolmer opened this issue Apr 26, 2024 · 3 comments
Labels
New feature Feature requests

Comments

@nwoolmer
Copy link
Contributor

nwoolmer commented Apr 26, 2024

Is your feature request related to a problem?

New features and bugfixes can lead to breaking changes. These are generally for the better, but can sometimes have a wide impact that can affect users in different ways.

This is highlighted where possible - PR titles, release notes, newsletter. Any migration info should be added to the docs at the relevant section. We also may provide config options to control the new behaviour, where appropriate.

Let's look at examples. Here's a narrower change:

#4277

In this case, no feature was gained or removed, but a default was changed. This only affected one style of query, and providing a config option is relatively low impact long term. The aim of the PR was to improve baseline performance for most users, so in general a strong upside.

#4443

In this PR, the impact is wider reaching, since it fundamentally changes the manner in which SQL queries are parsed. Each user could have breakages for any of their queries. In the long run, a config option to disable the changes is not ideal to maintain. Therefore, a temporary config option was added, which will allow users to delay the changes if they need to take the newest bug fixes. Such an option should be targeted for removal in the following version (i.e if in 7.4.3, gone in 7.4.4).

Overall, a clearer means to manage feature flags that's transparent to both users and maintainers could be useful, without over-complicating the situation or being too inflexible to user needs.

Describe the solution you'd like.

Some possible ideas

  • A more 'formal' way to handle temporary config options/feature flags
  • Deprecation of config options (logging warnings when an option is no longer in-use)
  • Some sort of compatibility mode, or checks to warn about the possible impact
  • Addition of breaking change notes to startup logs for the relevant version.

This is an issue for ideas and not a clear plan! We welcome comments and discussion on how best to approach this.

Describe alternatives you've considered.

No response

Full Name:

Nick Woolmer

Affiliation:

QuestDB

Additional context

No response

@nwoolmer nwoolmer added the New feature Feature requests label Apr 26, 2024
@jerrinot
Copy link
Contributor

jerrinot commented Apr 26, 2024

providing a config option is relatively low impact long term

I know @bluestreak01 has a different view, but I disagree with this sentiment. Config options are evil! :)

Each config option:

  1. Makes the product more complicated. Important options (say timeouts) or drowning in "compatibility switches" and feature flags.
  2. Increases testing surface. You should test different combinations, etc. It gets out of hand quickly.

This is not to say we should never introduce a feature flag or have a policy on it. I merely want to challenge the idea that "switches" and "feature flags" are cheap.

@nwoolmer
Copy link
Contributor Author

nwoolmer commented Apr 26, 2024

To clarify, what I meant to say was that flags with a narrow scope and (ideally) temporary are cheap, versus wide scope and permanent. I totally agree that we don't want to end up with a ball of feature flag mud!

For that particular feature (choosing whether you want to default to CALENDAR vs FIRST OBSERVATION alignment), it seemed low impact. Even this perhaps should have been a temporary option.

For the other PR, it changes the SQL parser so having a 'get-out' with a temporary flag seemed to be sensible. It means that we can revert the change before release if there's an issue, and we can release bugfixes to users even if they can't migrate yet. It should be removed in the following version except in extenuating circumstances.

Ideally, we wouldn't include any flag for most breaking changes, to keep things simple.

@jerrinot
Copy link
Contributor

I can agree with that :)

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

No branches or pull requests

2 participants