backupccl: send chunks with fail scatters to random node in generative ssp #97589
Conversation
|
This change is |
rhu713
changed the title
| log.Warningf(ctx, "scatter returned node 0. Route span starting at %s to current node %v because of hash error: %v", | ||
| scatterKey, nodeID, err) | ||
| } else { | ||
| randomNum := int(hash.Sum32()) |
There was a problem hiding this comment.
nit: call this variable a hashedKey instead?
| if nodeID, ok := flowCtx.NodeID.OptionalNodeID(); ok { | ||
| cachedNodeIDs := cache.cachedNodeIDs() | ||
| if len(cachedNodeIDs) > 0 && len(importSpanChunk.entries) > 0 { | ||
| hash := fnv.New32a() |
There was a problem hiding this comment.
nit: i don't think the memory footprint of a hash is that big, but we could instantiate it outside of the loop and reset it here.
| if nodeID, ok := flowCtx.NodeID.OptionalNodeID(); ok { | ||
| cachedNodeIDs := cache.cachedNodeIDs() | ||
| if len(cachedNodeIDs) > 0 && len(importSpanChunk.entries) > 0 { |
There was a problem hiding this comment.
nit: i don't think we need to check the length of importSpanChunk.entries, given that we grab the first entry above (line 360)
| testDiskMonitor := execinfra.NewTestDiskMonitor(ctx, st) | ||
| defer testDiskMonitor.Stop(ctx) | ||
|
|
||
| // Set up the test so that the test context is canceled after the first entry |
There was a problem hiding this comment.
i think this comment can be removed.
|
also, we should backport this to 23.1 |
…e ssp For chunks that have failed to scatter, this patch routes the chunk to a random node instead of the current node. This is necessary as prior to the generative version, split and scatter processors were on every node, thus there was no imbalance introduced from routing chunks that have failed to scatter to the current node. The new generative split and scatter processor is only on 1 node, and thus would cause the same node to process all chunks that have failed to scatter. Release note: None
|
bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 210650d to blathers/backport-release-23.1-97589: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
For chunks that have failed to scatter, this patch routes the chunk to a
random node instead of the current node. This is necessary as prior to the
generative version, split and scatter processors were on every node, thus there
was no imbalance introduced from routing chunks that have failed to scatter to
the current node. The new generative split and scatter processor is only on 1
node, and thus would cause the same node to process all chunks that have failed
to scatter.
Addresses run 6 and 9 of #99206
Release note: None