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

Mz clusters add introspection columns #27803

Merged

Conversation

jubrad
Copy link
Contributor

@jubrad jubrad commented Jun 21, 2024

Motivation

We should show these as they may cause replicas to be redeployed on alter cluster if changed.

Tips for reviewer

Checklist

@jubrad jubrad requested a review from jkosh44 June 21, 2024 17:37
@jubrad jubrad requested review from a team as code owners June 21, 2024 17:37
@jubrad jubrad force-pushed the mz_clusters-add-introspeection-columns branch from 6d95778 to 47a8c39 Compare June 21, 2024 17:37
Some(config.logging.log_logging),
config.logging.interval.map(|d| {
Interval::from_duration(&d)
.expect("planning ensured this convertible back to interval")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this, I stole it from the cluster_schedules table...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is fine. We create the Duration from an Interval in planning, so converting it back should always succeed.

Copy link
Contributor

@jkosh44 jkosh44 left a comment

Choose a reason for hiding this comment

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

LGTM, can you also update the docs:

### `mz_clusters`
The `mz_clusters` table contains a row for each cluster in the system.
<!-- RELATION_SPEC mz_catalog.mz_clusters -->
| Field | Type | Meaning |
|----------------------|----------------------|------------------------------------------------------------------------------------------------------------------------------------------|
| `id` | [`text`] | Materialize's unique ID for the cluster. |
| `name` | [`text`] | The name of the cluster. |
| `owner_id` | [`text`] | The role ID of the owner of the cluster. Corresponds to [`mz_roles.id`](/sql/system-catalog/mz_catalog/#mz_roles). |
| `privileges` | [`mz_aclitem array`] | The privileges belonging to the cluster. |
| `managed` | [`boolean`] | Whether the cluster is a [managed cluster](/sql/create-cluster/) with automatically managed replicas. |
| `size` | [`text`] | If the cluster is managed, the desired size of the cluster's replicas. `NULL` for unmanaged clusters. |
| `replication_factor` | [`uint4`] | If the cluster is managed, the desired number of replicas of the cluster. `NULL` for unmanaged clusters. |
| `disk` | [`boolean`] | **Unstable** If the cluster is managed, `true` if the replicas have the `DISK` option . `NULL` for unmanaged clusters. |
| `availability_zones` | [`text list`] | **Unstable** If the cluster is managed, the list of availability zones specified in `AVAILABILITY ZONES`. `NULL` for unmanaged clusters. |
.

You'll also need to wait for SQL council approval before merging.

src/catalog/src/builtin.rs Outdated Show resolved Hide resolved
Some(config.logging.log_logging),
config.logging.interval.map(|d| {
Interval::from_duration(&d)
.expect("planning ensured this convertible back to interval")
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is fine. We create the Duration from an Interval in planning, so converting it back should always succeed.

@jubrad jubrad force-pushed the mz_clusters-add-introspeection-columns branch from 47a8c39 to 0efe6e1 Compare June 21, 2024 17:58
@jubrad jubrad requested a review from morsapaes as a code owner June 21, 2024 17:58
@jubrad jubrad force-pushed the mz_clusters-add-introspeection-columns branch from 2b36022 to 73e5f1d Compare June 21, 2024 19:25
Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

One minor tweak on behalf of the SQL council. Thanks for doing this!

| `replication_factor` | [`uint4`] | If the cluster is managed, the desired number of replicas of the cluster. `NULL` for unmanaged clusters. |
| `disk` | [`boolean`] | **Unstable** If the cluster is managed, `true` if the replicas have the `DISK` option . `NULL` for unmanaged clusters. |
| `availability_zones` | [`text list`] | **Unstable** If the cluster is managed, the list of availability zones specified in `AVAILABILITY ZONES`. `NULL` for unmanaged clusters. |
| `introspection_logging` | [`boolean`] | Whether introspection data gather is logged. |
Copy link
Member

@benesch benesch Jun 22, 2024

Choose a reason for hiding this comment

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

We should match the public CREATE CLUSTER wording here (INTROSPECTION DEBUGGING), rather than the internal wording in the code.

@jubrad jubrad changed the title Mz clusters add introspeection columns Mz clusters add introspection columns Jun 24, 2024
@jubrad jubrad force-pushed the mz_clusters-add-introspeection-columns branch from 73e5f1d to fb27998 Compare June 24, 2024 01:26
@jubrad jubrad force-pushed the mz_clusters-add-introspeection-columns branch from fb27998 to 5098f2f Compare June 24, 2024 01:58
Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

LGTM from a SQL council perspective. 🎉 Thanks again for knocking this one out!

doc/user/content/sql/system-catalog/mz_catalog.md Outdated Show resolved Hide resolved
@jubrad jubrad force-pushed the mz_clusters-add-introspeection-columns branch from 5098f2f to 6315b82 Compare June 24, 2024 14:50
@jubrad jubrad force-pushed the mz_clusters-add-introspeection-columns branch from 6315b82 to 3a8ad39 Compare June 24, 2024 14:51
@jubrad jubrad merged commit 5a7ca9c into MaterializeInc:main Jun 24, 2024
80 checks passed
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.

3 participants