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

kvserver: remove StoreBenignError #129941

Closed
tbg opened this issue Aug 30, 2024 · 4 comments · Fixed by #139150
Closed

kvserver: remove StoreBenignError #129941

tbg opened this issue Aug 30, 2024 · 4 comments · Fixed by #139150
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-starter Might be suitable for a starter project for new employees or team members. T-kv KV Team

Comments

@tbg
Copy link
Member

tbg commented Aug 30, 2024

It doesn't seem to be used outside of a single test:

/pkg/kv/kvserver/benignerror/store_benign_error.go#L1-L32

// Copyright 2023 The Cockroach Authors.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package benignerror

import "github.com/cockroachdb/errors"

// StoreBenignError may be used for declaring an error that is less serious i.e.
// benign, originated from a store, and requires a retry. These errors are
// tracked in a metric away from other benign errors.
type StoreBenignError struct {
	cause error
}

// NewStoreBenign returns a new store benign error with the given error cause.
func NewStoreBenign(cause error) *StoreBenignError {
	return &StoreBenignError{cause: cause}
}

func (be *StoreBenignError) Error() string { return be.cause.Error() }
func (be *StoreBenignError) Cause() error  { return be.cause }

func IsStoreBenign(err error) bool {
	return errors.HasType(err, (*StoreBenignError)(nil))
}

Jira issue: CRDB-41787

@tbg tbg added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-starter Might be suitable for a starter project for new employees or team members. T-kv KV Team labels Aug 30, 2024
@XiaochenCui
Copy link
Contributor

Hi @tbg, are you working on this issue? I’d like to give it a try.

@tbg
Copy link
Member Author

tbg commented Sep 8, 2024

Hello! I'm not working on this - if you can produce a PR that needs only minimal feedback I'd be happy to merge it.

@XiaochenCui
Copy link
Contributor

Thanks. I noticed that the IsStoreBenign API is used in pkg/kv/kvserver/queue.go. I'm new to the "kvserver" package, so the PR might take a week to produce. We need to ensure that the modification doesn't break anything.

XiaochenCui added a commit to XiaochenCui/cockroach that referenced this issue Sep 8, 2024
Before commit 3f0b37a, the
StoreBenignError is used to handle pebble.ErrSnapshotExcised. As the
latter has been removed from pebble, we don't need StoreBenignError
anymore.

This commit does the following:

- Remove type "StoreBenignError".
- Remove the related increase action on counter "storeFailures".
- Update related tests "TestBaseQueueRequeue".

Fixes: cockroachdb#129941

Release note: None
@XiaochenCui
Copy link
Contributor

hi @tbg
I did some research and found it's ok to remove StoreBenignError, see this pr for details: #130308

XiaochenCui added a commit to XiaochenCui/cockroach that referenced this issue Sep 10, 2024
Before commit 3f0b37a, the
StoreBenignError is used to handle pebble.ErrSnapshotExcised. As the
latter has been removed from pebble, we don't need StoreBenignError
anymore.

This commit does the following:

- Remove type "StoreBenignError".
- Remove the related increase action on counter "storeFailures".
- Update related tests "TestBaseQueueRequeue".

Fixes: cockroachdb#129941

Release note: None
craig bot pushed a commit that referenced this issue Jan 21, 2025
139150: kvserver: remove StoreBenignError r=tbg a=tbg

Before commit 3f0b37a, the StoreBenignError is used to handle pebble.ErrSnapshotExcised. As the latter has been removed from pebble, we don't need StoreBenignError anymore.

This commit does the following:

- Remove type "StoreBenignError".
- Remove the related increase action on counter "storeFailures".
- Update related tests "TestBaseQueueRequeue".

Fixes: #129941

Closes: #130308

Release note: None

139280: roachtest: adding backup/restore tests for minio r=sravotto a=sravotto

Introducing a test to verify that we can backup and restore into a Minio Object Store cluster, using S3 API.

Fixes: #139272

Release note: None

139333: roachtest: only run 30 node backfill tests in full ac mode r=andrewbaptist a=andrewbaptist

In the non-full AC modes, a node can OOM during the fill period and the test will fail. This impacts the perturbation/metamorphic/backfill test.

Fixes: #139302
Informs: #139319

Release note: None

139475: rangefeed: fix test logging r=tbg a=stevendanna

The logging didn't actually print the value as it seemed to intend.

Informs #119340

Release note: None

Co-authored-by: XiaochenCui <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Silvano Ravotto <[email protected]>
Co-authored-by: Andrew Baptist <[email protected]>
Co-authored-by: Steven Danna <[email protected]>
@craig craig bot closed this as completed in 1d85eee Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-starter Might be suitable for a starter project for new employees or team members. T-kv KV Team
Projects
None yet
2 participants