Skip to content

Commit

Permalink
kv: refactor CompareWithLocality to use enum
Browse files Browse the repository at this point in the history 
Prior to this commit, `CompareWithLocality` returned two boolean values
indicating whether two localities were cross-region and cross-zone. However,
this required callers to perform additional cross-comparison of these boolean
values to make meaningful metrics updates.

To simplify this, this commit introduces a new enum type
`LocalityComparisonType`. It provides four locality comparison results: cross
region, same region cross zone, same region same zone, and undefined
(indicating error behavior). This refactoring allow the caller to directly use
the comparison result without additional operations.

In addition, this commit also updates the logic to classify activities between
different regions as cross-regional, regardless of the zone tiers’
configuration. Initially, cross-region but same-zone tiers activities were
flagged as misconfiguration. After some discussion, we have decided that
regions should be non-overlapping. Hence, same zone tiers from different
regions should still be considered as different zones.

Part of: #103983

Release note: None
  • Loading branch information
@wenyihu6
wenyihu6 committed Jun 21, 2023
1 parent b952eb1 commit cce98d1
Show file tree
Hide file tree
Showing 8 changed files with 189 additions and 195 deletions.
86 changes: 41 additions & 45 deletions pkg/kv/kvclient/kvcoord/dist_sender.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2296,12 +2296,12 @@ func (ds *DistSender) sendToReplicas(
if ds.BatchRequestInterceptor != nil {
ds.BatchRequestInterceptor(ba)
}
shouldIncCrossRegion, shouldIncCrossZone := ds.checkAndUpdateBatchRequestMetrics(ctx, ba)
comparisonResult := ds.checkAndUpdateCrossLocalityBatchMetrics(ctx, ba)
br, err = transport.SendNext(ctx, ba)
if ds.BatchResponseInterceptor != nil {
ds.BatchResponseInterceptor(br)
}
ds.checkAndUpdateBatchResponseMetrics(br, shouldIncCrossRegion, shouldIncCrossZone)
ds.updateCrossLocalityBatchMetrics(br, comparisonResult)
ds.maybeIncrementErrCounters(br, err)

if err != nil {
Expand Down Expand Up @@ -2557,71 +2557,67 @@ func (ds *DistSender) sendToReplicas(
}
}

// isCrossRegionCrossZoneBatch returns (bool, bool) - indicating if the given
// batch request is cross-region and cross-zone respectively.
func (ds *DistSender) isCrossRegionCrossZoneBatch(
// getCrossLocalityComparison compares the localities of the current node and
// the destination range node to determine if the given batch request is
// cross-region and cross-zone.
func (ds *DistSender) getCrossLocalityComparison(
ctx context.Context, ba *kvpb.BatchRequest,
) (bool, bool) {
) roachpb.LocalityComparisonType {
gatewayNodeDesc, err := ds.nodeDescs.GetNodeDescriptor(ba.GatewayNodeID)
if err != nil {
log.VEventf(ctx, 2, "failed to perform look up for node descriptor %s", err)
return false, false
return roachpb.LocalityComparisonType_UNDEFINED
}
destinationNodeDesc, err := ds.nodeDescs.GetNodeDescriptor(ba.Replica.NodeID)
if err != nil {
log.VEventf(ctx, 2, "failed to perform look up for node descriptor %s", err)
return false, false
return roachpb.LocalityComparisonType_UNDEFINED
}
isCrossRegion, regionErr, isCrossZone, zoneErr := gatewayNodeDesc.Locality.IsCrossRegionCrossZone(destinationNodeDesc.Locality)

comparisonResult, regionErr, zoneErr := gatewayNodeDesc.Locality.CompareWithLocality(destinationNodeDesc.Locality)
if regionErr != nil {
log.VEventf(ctx, 2, "%v", regionErr)
log.VEventf(ctx, 2, "unable to determine if batch is cross region %v", regionErr)
}
if zoneErr != nil {
log.VEventf(ctx, 2, "%v", zoneErr)
log.VEventf(ctx, 2, "unable to determine if batch is cross zone %v", zoneErr)
}
return isCrossRegion, isCrossZone
return comparisonResult
}

// checkAndUpdateBatchRequestMetrics updates the batch requests metrics in a
// more meaningful way. Cross-region metrics monitor activities across different
// regions. Cross-zone metrics monitor cross-zone activities within the same
// region or in cases where region tiers are not configured. The check result is
// returned here to avoid redundant check for metrics updates after receiving
// batch responses.
func (ds *DistSender) checkAndUpdateBatchRequestMetrics(
// checkAndUpdateCrossLocalityBatchMetrics updates the batch requests metrics in
// a more meaningful way. Cross-region metrics monitor activities across
// different regions. Cross-zone metrics monitor cross-zone activities within
// the same region or in cases where region tiers are not configured. The
// locality comparison result is returned here to avoid redundant check for
// metrics updates after receiving batch responses.
func (ds *DistSender) checkAndUpdateCrossLocalityBatchMetrics(
ctx context.Context, ba *kvpb.BatchRequest,
) (shouldIncCrossRegion bool, shouldIncCrossZone bool) {
) roachpb.LocalityComparisonType {
ds.metrics.ReplicaAddressedBatchRequestBytes.Inc(int64(ba.Size()))
isCrossRegion, isCrossZone := ds.isCrossRegionCrossZoneBatch(ctx, ba)
if isCrossRegion {
if !isCrossZone {
log.VEventf(ctx, 2, "unexpected: cross region but same zone")
} else {
ds.metrics.CrossRegionBatchRequestBytes.Inc(int64(ba.Size()))
shouldIncCrossRegion = true
}
} else {
if isCrossZone {
ds.metrics.CrossZoneBatchRequestBytes.Inc(int64(ba.Size()))
shouldIncCrossZone = true
}
}
return shouldIncCrossRegion, shouldIncCrossZone
comparisonResult := ds.getCrossLocalityComparison(ctx, ba)
switch comparisonResult {
case roachpb.LocalityComparisonType_CROSS_REGION:
ds.metrics.CrossRegionBatchRequestBytes.Inc(int64(ba.Size()))
case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE:
ds.metrics.CrossZoneBatchRequestBytes.Inc(int64(ba.Size()))
case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE:
// No metrics or error reporting.
}
return comparisonResult
}

// checkAndUpdateBatchResponseMetrics updates the batch response metrics based
// on the shouldIncCrossRegion and shouldIncCrossZone parameters. These
// parameters are determined during the initial check for batch requests. The
// underlying assumption is that if requests were cross-region or cross-zone,
// the response should be as well.
func (ds *DistSender) checkAndUpdateBatchResponseMetrics(
br *kvpb.BatchResponse, shouldIncCrossRegion bool, shouldIncCrossZone bool,
// updateCrossLocalityBatchMetrics updates the batch response metrics based on
// the comparisonResult parameter determined during the initial batch requests
// check. The underlying assumption is that the response should match the
// cross-region or cross-zone nature of the request.
func (ds *DistSender) updateCrossLocalityBatchMetrics(
br *kvpb.BatchResponse, comparisonResult roachpb.LocalityComparisonType,
) {
ds.metrics.ReplicaAddressedBatchResponseBytes.Inc(int64(br.Size()))
if shouldIncCrossRegion {
switch comparisonResult {
case roachpb.LocalityComparisonType_CROSS_REGION:
ds.metrics.CrossRegionBatchResponseBytes.Inc(int64(br.Size()))
}
if shouldIncCrossZone {
case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE:
ds.metrics.CrossZoneBatchResponseBytes.Inc(int64(br.Size()))
}
}
Expand Down
15 changes: 2 additions & 13 deletions pkg/kv/kvserver/allocator/storepool/store_pool.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1344,22 +1344,11 @@ func (sp *StorePool) GetNodeLocalityString(nodeID roachpb.NodeID) string {
return sp.getNodeLocalityWithString(nodeID).str
}

// getNodeLocality returns the locality information for the given node.
func (sp *StorePool) getNodeLocality(nodeID roachpb.NodeID) roachpb.Locality {
// GetNodeLocality returns the locality information for the given node.
func (sp *StorePool) GetNodeLocality(nodeID roachpb.NodeID) roachpb.Locality {
return sp.getNodeLocalityWithString(nodeID).locality
}

// IsCrossRegionCrossZone takes in two replicas and compares the locality of
// them based on their replica node IDs. It returns (bool, error, bool, error)
// where the boolean values indicate whether the two replicas' nodes are in
// different regions, different zones, along with any lookup errors.
func (sp *StorePool) IsCrossRegionCrossZone(
firstReplica roachpb.ReplicaDescriptor, secReplica roachpb.ReplicaDescriptor,
) (bool, error, bool, error) {
return sp.getNodeLocality(firstReplica.NodeID).IsCrossRegionCrossZone(
sp.getNodeLocality(secReplica.NodeID))
}

// IsStoreReadyForRoutineReplicaTransfer returns true iff the store's node is
// live (as indicated by its `NodeLivenessStatus`) and thus a legal candidate
// to receive a replica.
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/replica_command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3152,7 +3152,7 @@ func (r *Replica) followerSendSnapshot(
r.store.metrics.DelegateSnapshotSendBytes.Inc(inc)
}
r.store.metrics.RangeSnapshotSentBytes.Inc(inc)
r.store.updateCrossLocalitySnapshotMetrics(
r.store.checkAndUpdateCrossLocalitySnapshotMetrics(
ctx, req.CoordinatorReplica, req.RecipientReplica, inc, true /* isSent */)

switch header.Priority {
Expand Down
62 changes: 28 additions & 34 deletions pkg/kv/kvserver/store_snapshot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -973,58 +973,52 @@ func (s *Store) checkSnapshotOverlapLocked(
return nil
}

// shouldIncrementCrossLocalitySnapshotMetrics returns (bool, bool) - indicating
// if the two given replicas are cross-region and cross-zone respectively.
func (s *Store) shouldIncrementCrossLocalitySnapshotMetrics(
// getCrossLocalityComparison compares the localities of the two given replicas.
func (s *Store) getCrossLocalityComparison(
ctx context.Context, firstReplica roachpb.ReplicaDescriptor, secReplica roachpb.ReplicaDescriptor,
) (bool, bool) {
isCrossRegion, regionErr, isCrossZone, zoneErr := s.cfg.StorePool.IsCrossRegionCrossZone(
firstReplica, secReplica)
) roachpb.LocalityComparisonType {
firstLocality := s.cfg.StorePool.GetNodeLocality(firstReplica.NodeID)
secLocality := s.cfg.StorePool.GetNodeLocality(secReplica.NodeID)
comparisonResult, regionErr, zoneErr := firstLocality.CompareWithLocality(secLocality)
if regionErr != nil {
log.VEventf(ctx, 2, "unable to determine if snapshot is cross region %v", regionErr)
}
if zoneErr != nil {
log.VEventf(ctx, 2, "unable to determine if snapshot is cross zone %v", zoneErr)
}
return isCrossRegion, isCrossZone
return comparisonResult
}

// updateCrossLocalitySnapshotMetrics updates the snapshot metrics in a more
// meaningful way. Cross-region metrics monitor activities across different
// regions. Cross-zone metrics monitor any cross-zone activities within the same
// region or if the region tiers are not configured.
func (s *Store) updateCrossLocalitySnapshotMetrics(
// checkAndUpdateCrossLocalitySnapshotMetrics updates the snapshot metrics in
// a more meaningful way. Cross-region metrics monitor activities across
// different regions. Cross-zone metrics monitor cross-zone activities within
// the same region or in cases where region tiers are not configured.
func (s *Store) checkAndUpdateCrossLocalitySnapshotMetrics(
ctx context.Context,
firstReplica roachpb.ReplicaDescriptor,
secReplica roachpb.ReplicaDescriptor,
inc int64,
isSent bool,
) {
isCrossRegion, isCrossZone := s.shouldIncrementCrossLocalitySnapshotMetrics(ctx, firstReplica, secReplica)
comparisonResult := s.getCrossLocalityComparison(ctx, firstReplica, secReplica)
if isSent {
if isCrossRegion {
if !isCrossZone {
log.VEventf(ctx, 2, "unexpected: cross region but same zone")
} else {
s.metrics.RangeSnapShotCrossRegionSentBytes.Inc(inc)
}
} else {
if isCrossZone {
s.metrics.RangeSnapShotCrossZoneSentBytes.Inc(inc)
}
switch comparisonResult {
case roachpb.LocalityComparisonType_CROSS_REGION:
s.metrics.RangeSnapShotCrossRegionSentBytes.Inc(inc)
case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE:
s.metrics.RangeSnapShotCrossZoneSentBytes.Inc(inc)
case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE:
// No metrics or error reporting.
}
} else {
// isReceived
if isCrossRegion {
if !isCrossZone {
log.VEventf(ctx, 2, "unexpected: cross region but same zone")
} else {
s.metrics.RangeSnapShotCrossRegionRcvdBytes.Inc(inc)
}
} else {
if isCrossZone {
s.metrics.RangeSnapShotCrossZoneRcvdBytes.Inc(inc)
}
switch comparisonResult {
case roachpb.LocalityComparisonType_CROSS_REGION:
s.metrics.RangeSnapShotCrossRegionRcvdBytes.Inc(inc)
case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE:
s.metrics.RangeSnapShotCrossZoneRcvdBytes.Inc(inc)
case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE:
// No metrics or error reporting.
}
}
}
Expand Down Expand Up @@ -1145,7 +1139,7 @@ func (s *Store) receiveSnapshot(

recordBytesReceived := func(inc int64) {
s.metrics.RangeSnapshotRcvdBytes.Inc(inc)
s.updateCrossLocalitySnapshotMetrics(
s.checkAndUpdateCrossLocalitySnapshotMetrics(
ctx, header.RaftMessageRequest.FromReplica, header.RaftMessageRequest.ToReplica, inc, false /* isSent */)

switch header.Priority {
Expand Down
31 changes: 18 additions & 13 deletions pkg/roachpb/metadata.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -675,14 +675,11 @@ func (l Locality) getFirstRegionFirstZone() (
return firstRegionValue, hasRegion, firstZoneKey, firstZoneValue, hasZone
}

// IsCrossRegionCrossZone returns multiple values containing:
// 1. A boolean value indicating if this and the provided locality are
// cross-region.
// 2. Error indicating if either locality does not have a "region" tier key.
// 3. A boolean value indicating if this and the provided locality are
// cross-zone.
// 4. Error indicating if either locality does not have a "zone" tier key or if
// the first "zone" tier keys used by two localities are different.
// CompareWithLocality returns the comparison result between this and the
// provided other locality along with any lookup errors. Possible errors include
// 1. if either locality does not have a "region" tier key. 2. if either
// locality does not have a "zone" tier key or if the first "zone" tier keys
// used by two localities are different.
//
// Limitation:
// - It is unfortunate that the tier key is hardcoded here. Ideally, we would
Expand All @@ -699,14 +696,14 @@ func (l Locality) getFirstRegionFirstZone() (
// a single function to avoid overhead. If you are adding additional locality
// tiers comparisons, it is recommended to handle them within one tier list
// iteration.
func (l Locality) IsCrossRegionCrossZone(
func (l Locality) CompareWithLocality(
other Locality,
) (isCrossRegion bool, regionErr error, isCrossZone bool, zoneErr error) {
) (_ LocalityComparisonType, regionErr error, zoneErr error) {
firstRegionValue, hasRegion, firstZoneKey, firstZone, hasZone := l.getFirstRegionFirstZone()
firstRegionValueOther, hasRegionOther, firstZoneKeyOther, firstZoneOther, hasZoneOther := other.getFirstRegionFirstZone()

isCrossRegion = firstRegionValue != firstRegionValueOther
isCrossZone = firstZone != firstZoneOther
isCrossRegion := firstRegionValue != firstRegionValueOther
isCrossZone := firstZone != firstZoneOther

if !hasRegion || !hasRegionOther {
isCrossRegion = false
Expand All @@ -718,7 +715,15 @@ func (l Locality) IsCrossRegionCrossZone(
zoneErr = errors.Errorf("localities must have a valid zone tier key for cross-zone comparison")
}

return isCrossRegion, regionErr, isCrossZone, zoneErr
if isCrossRegion {
return LocalityComparisonType_CROSS_REGION, regionErr, zoneErr
} else {
if isCrossZone {
return LocalityComparisonType_SAME_REGION_CROSS_ZONE, regionErr, zoneErr
} else {
return LocalityComparisonType_SAME_REGION_SAME_ZONE, regionErr, zoneErr
}
}
}

// SharedPrefix returns the number of this locality's tiers which match those of
Expand Down
16 changes: 16 additions & 0 deletions pkg/roachpb/metadata.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,22 @@ message Tier {
optional string value = 2 [(gogoproto.nullable) = false];
}

// LocalityComparisonType represents different types of comparison results that
// indicate the relationship between two localities.
enum LocalityComparisonType {
// CROSS_REGION indicates that the two localities have different region tiers.
CROSS_REGION = 0;
// SAME_REGION_CROSS_ZONE indicates that the two localities have the same
// region tiers but different zone tiers.
SAME_REGION_CROSS_ZONE = 1;
// SAME_REGION_SAME_ZONE indicates that the two localities have same region
// and same zone tiers.
SAME_REGION_SAME_ZONE = 2;
// UNDEFINED represents an undefined comparison result, indicating error
// behavior.
UNDEFINED = 3;
}

message Version {
option (gogoproto.equal) = true;
option (gogoproto.goproto_stringer) = false;
Expand Down
Loading

0 comments on commit cce98d1

Please sign in to comment.