-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
release-24.3: row: mark an error from streamer as StorageError #136060
Open
blathers-crl
wants to merge
1
commit into
release-24.3
Choose a base branch
from
blathers/backport-release-24.3-135859
base: release-24.3
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
Earlier this year we changed the vectorized panic-catcher, namely, we now check a few special error types (like `StorageError`, `notInternalError`, and `internalError` upfront - to avoid more expensive stack-based checks). If we find an `internalError` _and_ it doesn't have a PG code and doesn't have `StorageError` or `notInternalError` in the causes, we now always mark the error as assertion failure. This made it so that we started receiving more sentry reports than previously; in particular, whenever we're using the streamer API, we could now get `stop.ErrUnavailable` via the following sequence of calls `ColIndexJoin.Next` -> `cFetcher.StartScan` -> `txnKVStreamer.SetupNextFetch` -> `Streamer.Enqueue`, and this would trigger a sentry report. This commit fixes this by marking an error coming from `Streamer.Enqueue` as `StorageError` which will by-pass the logic in the panic-catcher. Release note: None
blathers-crl
bot
force-pushed
the
blathers/backport-release-24.3-135859
branch
from
November 23, 2024 00:51
52cab62
to
dc318bb
Compare
blathers-crl
bot
requested review from
mgartner
and removed request for
a team
November 23, 2024 00:51
blathers-crl
bot
added
blathers-backport
This is a backport that Blathers created automatically.
O-robot
Originated from a bot.
labels
Nov 23, 2024
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
blathers-crl
bot
added
the
backport
Label PR's that are backports to older release branches
label
Nov 23, 2024
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
backport
Label PR's that are backports to older release branches
blathers-backport
This is a backport that Blathers created automatically.
O-robot
Originated from a bot.
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.
Backport 1/1 commits from #135859 on behalf of @yuzefovich.
/cc @cockroachdb/release
Earlier this year we changed the vectorized panic-catcher, namely, we now check a few special error types (like
StorageError
,notInternalError
, andinternalError
upfront - to avoid more expensive stack-based checks). If we find aninternalError
and it doesn't have a PG code and doesn't haveStorageError
ornotInternalError
in the causes, we now always mark the error as assertion failure. This made it so that we started receiving more sentry reports than previously; in particular, whenever we're using the streamer API, we could now getstop.ErrUnavailable
via the following sequence of callsColIndexJoin.Next
->cFetcher.StartScan
->txnKVStreamer.SetupNextFetch
->Streamer.Enqueue
, and this would trigger a sentry report. This commit fixes this by marking an error coming fromStreamer.Enqueue
asStorageError
which will by-pass the logic in the panic-catcher.Fixes: #128649.
Release note: None
Release justification: low-risk bug fix.