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

Runtime: Set instance config with variables #4521

Merged
merged 9 commits into from
Apr 9, 2024

Conversation

begelundmuller
Copy link
Contributor

@begelundmuller begelundmuller commented Apr 5, 2024

  • Adds support for setting instance-wide config using variables.
  • Variables for instance config are prefixed with rill.. For example, rill.models.default_materialize configures the instance's default model materialization.
  • Only config properties that are safe for users to customize can be set with variables.
  • Makes previously hard-coded config constants configurable. The new list of configurable variables is:
    • rill.download_row_limit (int, default: 200k)
    • rill.pivot_cell_limit (int, default: 2m)
    • rill.interactive_sql_row_limit (int, default: 10k)
    • rill.stage_changes (bool, default: true)
    • rill.models.default_materialize (bool, default: false)
    • rill.models.materialize_delay_seconds (int, default: 0)
    • rill.alerts.default_streaming_refresh_cron (string, default: every 10 mins)

Closes #4426

@begelundmuller begelundmuller self-assigned this Apr 5, 2024
@begelundmuller begelundmuller marked this pull request as ready for review April 5, 2024 11:15
Copy link
Member

@k-anshul k-anshul left a comment

Choose a reason for hiding this comment

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

LGTM.
But I am not sure about the usage of rill. prefix here.
In general either we should allow all configuration(including connector related) to be appended with rill. as in the case of spark configurations : https://spark.apache.org/docs/latest/configuration.html#available-properties
or nothing should be appended and as in the case of duckdb.
As a user I would prefer if configurations are either rill.models.default_materialize,rill.connector.duckdb.path or models.default_materialize, connector.duckdb.path.

runtime/drivers/registry.go Outdated Show resolved Hide resolved
@begelundmuller
Copy link
Contributor Author

LGTM. But I am not sure about the usage of rill. prefix here. In general either we should allow all configuration(including connector related) to be appended with rill. as in the case of spark configurations : https://spark.apache.org/docs/latest/configuration.html#available-properties or nothing should be appended and as in the case of duckdb. As a user I would prefer if configurations are either rill.models.default_materialize,rill.connector.duckdb.path or models.default_materialize, connector.duckdb.path.

I understand this point. One issue is that users can also provide their own variables (for example, see https://github.com/rilldata/margin-metering/blob/main/rill.yaml). So to avoid conflicts, having a fixed prefix like rill. is necessary.

Then about connector. vs. rill.connector., yeah it might make sense to change it. One thing that makes it a little tricky is that the connectors are somewhat user defined – for example rill.connector.ezcommerce-slack.bot_token=... looks a little weird with the rill. prefix, since ezcommerce-slack is not Rill system related.

@k-anshul
Copy link
Member

k-anshul commented Apr 9, 2024

I understand this point. One issue is that users can also provide their own variables (for example, see https://github.com/rilldata/margin-metering/blob/main/rill.yaml). So to avoid conflicts, having a fixed prefix like rill. is necessary.

Makes sense.

Then about connector. vs. rill.connector., yeah it might make sense to change it. One thing that makes it a little tricky is that the connectors are somewhat user defined – for example rill.connector.ezcommerce-slack.bot_token=... looks a little weird with the rill. prefix, since ezcommerce-slack is not Rill system related.

May be we can consider connector.ezcommerce-slack.bot_token as dynamically generated rill variables.

@begelundmuller begelundmuller merged commit 121c0b5 into main Apr 9, 2024
7 checks passed
@begelundmuller begelundmuller deleted the begelundmuller/instance-config-with-vars branch April 9, 2024 12:27
AdityaHegde pushed a commit that referenced this pull request Apr 9, 2024
* Runtime: Set instance config with variables

* Add key for download row limit

* Config for pivot cell limit

* Fix test and lint

* Self review

* Self review

* Add more config keys

* Review
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.

Runtime: Project-wide config using variables
2 participants