catalog: Improve fencing #29224
Merged
catalog: Improve fencing #29224
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jkosh44
force-pushed
the
catalog-fencing
branch
13 times, most recently
from
8a7da69
to
5d550c9
Compare
Previously, the durable catalog could be fenced by one of the
following ways:
- It tried to listen to the durable catalog and saw a higher epoch.
- It tried to listen to the durable catalog and saw a higher binary
version.
- It tried to write to the durable catalog and had an upper
mismatch.
- It tried to write to the catalog migration shard and had an upper
mismatch.
These would either result in a non-descriptive
`DurableCatalogError::Fence` error or a
`DurableCatalogError::IncompatiblePersistVersion` error. Additionally,
if multiple of these conditions happened at the same time, it was
non-deterministic which error would be returned.
This commit modifies the conditions for when the durable catalog can be
fenced to the following conditions:
- It tried to listen to the durable catalog and saw a higher deploy
generation.
- It tried to listen to the durable catalog and saw a higher epoch.
- It tried to write to the durable catalog shard and had an upper
mismatch.
- It tried to write to the catalog migration shard and had an upper
mismatch.
When initializing the catalog, we still check that the current binary
version is not less than the catalog binary version, but we no longer
check during every update. After the catalog has been initialized,
another catalog can only increase the binary version by also fencing
the current catalog by another method.
This commit also creates a new error enum to explicitly codify these
different types of fences. When multiple of these conditions happen at
the same time, then we deterministically will return a fence error with
the priority of how the conditions are ordered above. The context of
the fence is useful to higher layers in determining what action to
take. For example if the deploy generation has increased, then we'd
like to gracefully exit with exit code 0 and let the new instance take
over.
Works towards resolving #29199
jkosh44
changed the title
jkosh44
force-pushed
the
catalog-fencing
branch
from
5d550c9
to
576da84
Compare
jkosh44
commented
Aug 27, 2024
Comment on lines
+154
to
+160
| // It's very likely that if we were to read the latest updates after this error, then we'd see | ||
| // a higher epoch/deploy generation from the other writer and get fenced through that. We could | ||
| // potentially always do that and eliminate this fence variant. However, the catalog debug tool | ||
| // is able to write to the catalog without incrementing the epoch and therefore would not be | ||
| // able to fence this instance. It's technically possible for this instance to survive and | ||
| // respond to a write from the catalog debug tool, but until more thought is put into it, the | ||
| // safest thing to do is crash and reboot. |
There was a problem hiding this comment.
I've realized that this isn't actually true. It's possible for the following to happen:
- Catalog opens.
- Catalog debug tool writes something.
- Catalog performs a read, which first involves listening to all changes in the shard, including the changes written by the debug tool. This also updates the internal upper.
- Catalog goes to make a write, it's already updated it's internal upper so no upper mismatch happens.
There was a problem hiding this comment.
This is an existing issue though so I'll address it in a follow up.
jkosh44
commented
Aug 27, 2024
| @@ -777,6 +792,7 @@ impl UnopenedPersistCatalogState { | |||
| persist_client: PersistClient, | |||
| organization_id: Uuid, | |||
| version: semver::Version, | |||
| deploy_generation: Option<u64>, | |||
There was a problem hiding this comment.
This one line is the reason the LoC is so high on this PR. We used to pass in the deploy generation when opening the durable catalog. Now we pass it in even earlier, before opening the catalog when we initialize it, which is handled by this function. This changed the API of a handful of heavily used methods.
The reason is so that we can fence the catalog by deploy version, before we even open the catalog. Maybe that was unnecessary and we should have kept the API as it was. This feels a bit safer for some reason.
aljoscha
approved these changes
Aug 27, 2024
src/catalog/src/durable/error.rs
Outdated
| #[error( | ||
| "builtin table migration shard upper {expected_upper:?} fenced by new builtin table migration shard upper {actual_upper:?}" | ||
| )] | ||
| Migration { |
There was a problem hiding this comment.
ignore if you got better things to do, but this could be MigrationUpper to match Upper
Co-authored-by: Aljoscha Krettek <mail@aljoscha.dev>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the durable catalog could be fenced by one of the
following ways:
version.
mismatch.
mismatch.
These would either result in a non-descriptive
DurableCatalogError::Fenceerror or aDurableCatalogError::IncompatiblePersistVersionerror. Additionally,if multiple of these conditions happened at the same time, it was
non-deterministic which error would be returned.
This commit modifies the conditions for when the durable catalog can be
fenced to the following conditions:
generation.
mismatch.
mismatch.
When initializing the catalog, we still check that the current binary
version is not less than the catalog binary version, but we no longer
check during every update. After the catalog has been initialized,
another catalog can only increase the binary version by also fencing
the current catalog by another method.
This commit also creates a new error enum to explicitly codify these
different types of fences. When multiple of these conditions happen at
the same time, then we deterministically will return a fence error with
the priority of how the conditions are ordered above. The context of
the fence is useful to higher layers in determining what action to
take. For example if the deploy generation has increased, then we'd
like to gracefully exit with exit code 0 and let the new instance take
over.
Works towards resolving MaterializeInc/database-issues#8474
Motivation
This PR fixes a recognized bug.
Tips for reviewer
I would recommend reviewing the files in the following order:
error.rspersist.rsdurable.rsenvironmentd/src/lib.rspreflight.rsbuiltin_item_migrations.rscatalog-debug/src/main.rscatalog.rsChecklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.