main, view: Pair view builder drain with its start #21909
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
The patch solved a problem related to an initialization order, but we
ran into another one -- after moving the initialization of group0_service,
it ended up being destroyed AFTER the CDC generation service would.
Since CDC generations are accessed in `storage_service::topology_state_load()`:
```
for (const auto& gen_id : _topology_state_machine._topology.committed_cdc_generations) {
rtlogger.trace("topology_state_load: process committed cdc generation {}", gen_id);
co_await _cdc_gens.local().handle_cdc_generation(gen_id);
```
we started getting the following failure:
```
Service &seastar::sharded<cdc::generation_service>::local() [Service = cdc::generation_service]: Assertion `local_is_initialized()' failed.
```
We're reverting the patch to go back to a more stable version of Scylla
and in the following commit, we'll solve the original issue in a more
systematic way.
This reverts commit 7bad837.
🔴 CI State: FAILUREBuild Details:
|
🔴 CI State: FAILURE✅ - Build Failed Tests (1/36079):Build Details:
|
|
Looks like #21628. Rerunning CI |
🟢 CI State: SUCCESS✅ - Build Build Details:
|
piodul
reviewed
Dec 17, 2024
db/view/view.cc
Outdated
Comment on lines
2098
to
2104
| future<> view_builder::stop() { | ||
| vlogger.info("Stopping view builder"); | ||
| return drain(); | ||
| co_return; | ||
| } |
There was a problem hiding this comment.
Nit: put an assertion here which checks that drain() was indeed run (it needs to be run in order for the view builder to be properly stopped) - for example, you could check if _as.abort_requested() is true
Before these changes, we only drained the view builder when stopping it, which was paired with its *construction*. However, since the view builder depends on group0_service, which is initialized AFTER the view builder is constructed, that could lead to segmentation faults. To amend it, we pair draining the view builder with its start, while leaving the lambda destroying it paired with its construction. This way we should have a proper initialization order, avoid bugs, and reinforce our understanding of how Scylla starts up. Fixes scylladb#20772
|
Version 2:
|
🔴 CI State: FAILURE✅ - Build Failed Tests (2/36177):Build Details:
|
Rerunning CI |
🔴 CI State: FAILURE✅ - Build Failed Tests (1/36344):Build Details:
|
I believe it's #15715. Retriggering CI |
🟢 CI State: SUCCESS✅ - Build Build Details:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR reverts a patch that solved a problem related to an initialization
order. Unfortunately, after merging it, we ran into another one -- after
changing the place where group0_service was initialized, it ended up
being destroyed AFTER the CDC generation service would. Since CDC
generations are accessed in
storage_service::topology_state_load():we started getting the following failure:
To solve the issue, we revert the changes and approach the problem
in another way: before that patch, we only drained the view builder
when stopping it, which was paired with its construction. However,
since the view builder depends on group0_service, which is initialized
AFTER the view builder is constructed, that could lead to segmentation
faults. To amend it, we pair draining the view builder with its start, while
leaving the lambda destroying it paired with its construction. This way
we should have a proper initialization order, avoid bugs, and reinforce
our understanding of how Scylla starts up.
Backport: The patch I'm reverting made it to 6.2, so we want to backport this one there too.
Fixes #20772
Fixes #21534