multitenant: handle missing NodeDescriptor in crdb_internal.ranges_no_leases #93127
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @ecwall, and @rafiss)
pkg/sql/crdb_internal.go line 3728 at r1 (raw file):
// Use empty string if nodeDesc is not found. // See https://github.com/cockroachdb/cockroach/issues/92915. if err == nil {
Not all errors indicate the desc was not found. Some errors should be propagated. Please use a more precise predicate here. Something like:
if err != nil {
if !xxx(err) {
return err
}
} else {
replicaLocalityString = nodeDesc.Locality.String()
}
There was a problem hiding this comment.
Nice, thanks for the quick fix! To confirm, the linked roachtest passes with this patch, right?
Reviewed all commit messages.
Reviewable status:
pkg/sql/crdb_internal.go line 3726 at r1 (raw file):
var replicaLocalityString string nodeDesc, err := p.ExecCfg().NodeDescs.GetNodeDescriptor(replica.NodeID) // Use empty string if nodeDesc is not found.
nit: could you expand on this comment a bit to say that we don't want this table to not render if locality information isn't available for a particular node for some reason?
|
@arulajmani @knz getting a "Sorry, you've run out of GitHub API quota." error in reviewable, but I addressed your comments. Working on running |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go line 3726 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: could you expand on this comment a bit to say that we don't want this table to not render if locality information isn't available for a particular node for some reason?
Updated.
pkg/sql/crdb_internal.go line 3728 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Not all errors indicate the desc was not found. Some errors should be propagated. Please use a more precise predicate here. Something like:
if err != nil { if !xxx(err) { return err } } else { replicaLocalityString = nodeDesc.Locality.String() }
Updated.
There was a problem hiding this comment.
Reviewed 10 of 13 files at r3, all commit messages.
Reviewable status:
pkg/util/errorutil/descriptor.go line 20 at r3 (raw file):
) const descriptorNotFound = "unable to look up descriptor for"
The API here is fine, but the implementation is not. We don't use Error() comparisons any more in CockroachDB.
Try this:
type descriptorNotFound struct{msg string}
func (e *descriptorNotFound) Error() string { return e.msg }pkg/util/errorutil/descriptor.go line 23 at r3 (raw file):
func IsDescriptorNotFoundError(err error) bool { return strings.HasPrefix(err.Error(), descriptorNotFound)
return errors.HasType(err, (*descriptorNotFound)(nil))
pkg/util/errorutil/descriptor.go line 27 at r3 (raw file):
func NewNodeNotFoundError(nodeID roachpb.NodeID) error { return errors.Errorf("%s n%d", descriptorNotFound, nodeID)
return &descriptorNotFound{fmt.Sprintf("unable to look up descriptor for n%d", nodeID")}
pkg/util/errorutil/descriptor.go line 31 at r3 (raw file):
func NewStoreNotFoundError(storeID roachpb.StoreID) error { return errors.Errorf("%s storeID %d", descriptorNotFound, storeID)
ditto & reuse the original error message for stores
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go line 3726 at r1 (raw file):
Previously, ecwall (Evan Wall) wrote…
Updated.
Arul: what do you think, should we render NULL in that case?
There was a problem hiding this comment.
Reviewable status:
pkg/util/errorutil/descriptor.go line 20 at r3 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
The API here is fine, but the implementation is not. We don't use
Error()comparisons any more in CockroachDB.Try this:
type descriptorNotFound struct{msg string} func (e *descriptorNotFound) Error() string { return e.msg }
Done.
pkg/util/errorutil/descriptor.go line 23 at r3 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
return
errors.HasType(err, (*descriptorNotFound)(nil))
Done.
pkg/util/errorutil/descriptor.go line 27 at r3 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
return &descriptorNotFound{fmt.Sprintf("unable to look up descriptor for n%d", nodeID")}
Done.
pkg/util/errorutil/descriptor.go line 31 at r3 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
ditto & reuse the original error message for stores
Done.
There was a problem hiding this comment.
LGTM but let's let arul finalize the review
Reviewed 2 of 13 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status:
|
I'm getting a different error trying to reproduce this: |
There was a problem hiding this comment.
LGTM once we've confirmed the roachtest passes.
Reviewed 10 of 13 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status:
pkg/sql/crdb_internal.go line 3726 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Arul: what do you think, should we render NULL in that case?
I think it's a good idea -- that way, we aren't conflating no locality string with one that couldn't be found.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go line 3726 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I think it's a good idea -- that way, we aren't conflating no locality string with one that couldn't be found.
Done.
…_leases Fixes #92915 This change matches the previous behavior of using "" for locality if the NodeDescriptor is not found instead of returning an error when generating crdb_internal.ranges_no_leases. Release note: None
|
bors r=arulajmani |
|
Build succeeded: |
Fixes #92915
This change matches the previous behavior of using "" for locality if the NodeDescriptor is not found instead of returning an error when generating crdb_internal.ranges_no_leases.
Release note: None