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

add pending field to managed cluster replicas #27808

Merged

Conversation

jubrad
Copy link
Contributor

@jubrad jubrad commented Jun 22, 2024

Motivation

More prep for graceful reconfig.

Reconfiguration replicas that come up will have a suffixed with -pending and will have a pending field in the cluster replica config.
https://github.com/MaterializeInc/database-issues/issues/5976

Tips for reviewer

Checklist

@jubrad jubrad force-pushed the feature/graceful-reconfig-replica-pending branch 3 times, most recently from 367ee40 to e3a3101 Compare June 24, 2024 17:36
@jubrad jubrad marked this pull request as ready for review June 24, 2024 18:59
@jubrad jubrad requested review from a team as code owners June 24, 2024 18:59
@jubrad jubrad requested a review from jkosh44 June 24, 2024 18:59
@jkosh44
Copy link
Contributor

jkosh44 commented Jun 24, 2024

Did someone on @MaterializeInc/adapter review the design doc for this? If so, they'd probably be a better reviewer than me.

@maddyblue
Copy link
Contributor

I'll review!

@maddyblue maddyblue requested review from maddyblue and removed request for jkosh44 June 24, 2024 20:58
src/controller/src/clusters.rs Outdated Show resolved Hide resolved
src/adapter/src/catalog/builtin_table_updates.rs Outdated Show resolved Hide resolved
src/adapter/src/catalog/builtin_table_updates.rs Outdated Show resolved Hide resolved
src/catalog/src/durable/upgrade/v58_to_v59.rs Outdated Show resolved Hide resolved
src/catalog/src/durable/upgrade/v58_to_v59.rs Outdated Show resolved Hide resolved
src/catalog/src/durable/upgrade/v58_to_v59.rs Outdated Show resolved Hide resolved
src/catalog/src/durable/upgrade/v58_to_v59.rs Outdated Show resolved Hide resolved
@jubrad jubrad force-pushed the feature/graceful-reconfig-replica-pending branch from e3a3101 to 6de244f Compare June 25, 2024 01:18
@jubrad jubrad force-pushed the feature/graceful-reconfig-replica-pending branch from 6de244f to 406bb18 Compare June 25, 2024 01:22
Copy link

shepherdlybot bot commented Jun 25, 2024

Risk Score:79 / 100 Bug Hotspots:1 Resilience Coverage:50%

Mitigations

Completing required mitigations increases Resilience Coverage.

  • (Required) Code Review 🔍 Detected
  • (Required) Observability
  • Feature Flag
  • Integration Test
  • QA Review 🔍 Detected
  • Run Nightly Tests
  • Unit Test 🔍 Detected
Risk Summary:

The pull request poses a high risk with a score of 79, indicating a significant likelihood of introducing a bug. This assessment is driven by predictors such as the sum of bug reports in the files changed and the delta of executable lines. Notably, there has been a historical trend showing that pull requests with these characteristics are 108% more likely to cause a bug compared to the repository's baseline. Additionally, the repository's predicted bug trend is on the rise, and there is at least one file modified that has a high frequency of recent bug fixes.

Note: The risk score is not based on semantic analysis but on historical predictors of bug occurrence in the repository. The attributes above were deemed the strongest predictors based on that history. Predictors and the score may change as the PR evolves in code, time, and review activity.

Bug Hotspots:
What's This?

File Percentile
../catalog/state.rs 99

Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

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

Nightly lgtm: https://buildkite.com/materialize/nightly/builds/8211#_
Coverage only shows upgrade tests missing (expected): https://buildkite.com/materialize/coverage/builds/430
I'll add a commit to test persisting the pending field.

Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

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

Ok, I see, this is actually just a new column in mz_cluster_replicas. I'm wondering why test/sqllogictest/autogenerated/mz_catalog.slt's SELECT position, name, type FROM objects WHERE schema = 'mz_catalog' AND object = 'mz_cluster_replicas' ORDER BY position didn't have to be changed? It seem like we should document and check the new fields too.

@jubrad
Copy link
Contributor Author

jubrad commented Jun 25, 2024

@def- this doesn't yet expose the field as a column in mz_cluster_replicas. Really there should be no functional changes from this pr, just a new field being added to the catalog that for now is always false.

Eventually we will expose it, but we'll want to wait for the rest of graceful reconfig functionality before we do that.

Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

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

All test failures are unrelated flakes, fixed in main.

@jubrad jubrad merged commit 6c0e475 into MaterializeInc:main Jun 26, 2024
193 of 196 checks passed
@jubrad jubrad deleted the feature/graceful-reconfig-replica-pending branch June 26, 2024 18:19
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.

4 participants