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 flag to support the legacy column style #379

Merged
merged 5 commits into from
Jul 12, 2024
Merged

Conversation

istreeter
Copy link
Contributor

@istreeter istreeter commented Jul 5, 2024

In v1 of BigQuery Loader, we created a separate column for each minor version of an Iglu schema. Whereas in v2 of BigQuery Loader, schemas with the same major version share the same column. This might make it hard for users to upgrade from version 1 to version 2; and so users might miss out on all the other lovely stuff we added in v2.

This PR adds back in support for the legacy style of columns. It is enabled by setting legacyColumns = [...] in the config file, where the config value lists schema keys to treat as legacy.

@istreeter istreeter force-pushed the legacy-columns-in-v2 branch from 82cd777 to 212b7f0 Compare July 5, 2024 15:41
In v1 of BigQuery Loader, we created a separate column for each _minor_
version of an Iglu schema.  Whereas in v2 of BigQuery Loader, schemas
with the same major version share the same column.  This might make it
hard for users to upgrade from version 1 to version 2; and so users
might miss out on all the other lovely stuff we added in v2.

This PR adds back in support for the legacy style of columns.  It is
enabled by setting `legacyColumns = true` in the config file.
@istreeter istreeter force-pushed the legacy-columns-in-v2 branch from 212b7f0 to 8465a17 Compare July 5, 2024 15:47
@istreeter istreeter marked this pull request as ready for review July 11, 2024 15:03
config/config.kinesis.reference.hocon Show resolved Hide resolved
# -- For these columns, there is a column per _minor_ version of each schema.
"legacyColumns": [
"iglu:com.acme/legacy/jsonschema/1-*-*"
"iglu:com.acme/legacy/jsonschema/2-*-*"
Copy link
Contributor

Choose a reason for hiding this comment

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

With first version we had a boolean toggle where we could just say 'I don't want v2 for now, just give me v1 for all columns'. Now If I want that behaviour, I have to list all my schemas here (well at least all names with *-*-* version pattern)?

I wonder if such toggle to disable v2 entirely could still be useful from user point of view. Maybe migration would be easier if we do the opposite - make v2Schemas configurable and all legacy by default...? Or some combination of boolean toggle + legacy schema list config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stanch please could you comment on this idea?

I implemented it the current way based on a conversation we had. So users are forced to start using the new column style if they track a new entity. Based on that plan, I think there is no need to add a flag which turns on v1-by-default. And I liked that plan because it keeps the implementation simpler.

But now is the time to comment if we do want to allow v1-by-default for a subset of users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually @pondzix I don't think we need to wait for Nick's comment. I am confident this is implemented exactly as we discussed, and as written about in some internal document.

If closer to release we decide to implement v1-by-default as an option, then we can do so as an extra feature request.

@istreeter istreeter merged commit 819f267 into v2 Jul 12, 2024
2 checks passed
@istreeter istreeter deleted the legacy-columns-in-v2 branch July 12, 2024 13:27
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.

2 participants