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

Add PartitionInstanceRing.GetReplicationSetForPartitionAndOperation() #623

Merged

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Dec 2, 2024

What this PR does:

Add PartitionInstanceRing.GetReplicationSetForPartitionAndOperation(). This PR is against usage-tracker branch, which is a branch me and @colega are using to build a prototype. This will not be merged to main for now.

Notes to reviewers:

  • Review with "hide whitespace changes"

Ensuring no regression on the already-existing GetReplicationSetsForOperation():

goos: darwin
goarch: arm64
pkg: github.com/grafana/dskit/ring
cpu: Apple M3 Pro
                                                        │ before.txt  │             after.txt              │
                                                        │   sec/op    │   sec/op     vs base               │
PartitionInstanceRing_GetReplicationSetsForOperation-11   21.81µ ± 0%   22.42µ ± 1%  +2.80% (p=0.000 n=10)

                                                        │  before.txt  │            after.txt             │
                                                        │     B/op     │     B/op      vs base            │
PartitionInstanceRing_GetReplicationSetsForOperation-11   39.12Ki ± 0%   39.12Ki ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

                                                        │ before.txt │           after.txt            │
                                                        │ allocs/op  │ allocs/op   vs base            │
PartitionInstanceRing_GetReplicationSetsForOperation-11   101.0 ± 0%   101.0 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

Which issue(s) this PR fixes:

N/A

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

// GetReplicationSetForPartitionAndOperation returns a ReplicationSet for the input partition. If the partition doesn't
// exist or there are no healthy owners for the partition, an error is returned.
func (r *PartitionInstanceRing) GetReplicationSetForPartitionAndOperation(partitionID int32, op Operation) (ReplicationSet, error) {
zonesBuffer := make([]string, 0, 3) // Pre-allocate buffer assuming 3 zones.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid the allocation here completely.

Suggested change
zonesBuffer := make([]string, 0, 3) // Pre-allocate buffer assuming 3 zones.
var stackZonesBuffer [3]string // Pre-allocate buffer assuming 3 zones.
zonesBuffer := stackZonesBuffer[:]

(I'd also do 5, not 3, let's not be greedy)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picked the on-stack buffer: bf3dc4a. About 3 vs 5 I will leave it for a separate discussion, given in the ring/ pkg we assume 3 in different places.

Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, much easier to review with whitespace ignored.

Signed-off-by: Marco Pracucci <[email protected]>
@pracucci pracucci merged commit c915e15 into usage-tracker Dec 4, 2024
5 checks passed
@pracucci pracucci deleted the usage-tracker-add-per-partition-replication-set-lookup branch December 4, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants