REP-6492 Switch to $sampleRate-style partitioning #128
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FGasper
force-pushed
the
REP-6492-samplerate-partition
branch
from
180b79a to
ab5b493
Compare
tdq45gj
requested changes
Aug 21, 2025
tdq45gj
reviewed
Aug 21, 2025
Comment on lines
92
to
103
| /* | ||
| _, err := verifier.verificationTaskCollection().InsertOne( | ||
| ctx, | ||
| task, | ||
| ) | ||
| require.NoError(err, "should insert collection task") | ||
|
|
||
| require.NoError( | ||
| verifier.ProcessCollectionVerificationTask(ctx, 0, task), | ||
| "task should succeed", | ||
| ) | ||
| */ |
There was a problem hiding this comment.
Do we still need this in the test?
internal/verifier/partition.go
Outdated
| @@ -95,8 +95,9 @@ func (verifier *Verifier) createPartitionTasksWithSampleRateRetryable( | |||
| } | |||
|
|
|||
| pipeline := mongo.Pipeline{ | |||
| {{"$project", bson.D{{"_id", 1}}}}, | |||
| // NB: $sort MUST precede $project in order to avoid a blocking sort. | |||
There was a problem hiding this comment.
Could be more specific that it's only a problem for versions before v6.0.
internal/verifier/partition.go
Outdated
Comment on lines
200
to
201
| idealNumPartitions := util.Divide(collBytes, idealPartitionBytes) | ||
| docsPerPartition := util.Divide(docsCount, idealNumPartitions) |
There was a problem hiding this comment.
It seems that it's still possible to get division by 0 error. If collBytes < idealPartitionBytes, idealNumPartitions would become 0. Could we limit idealNumPartitions and docsPerPartition to be larger than 1?
Suggested change
| idealNumPartitions := util.Divide(collBytes, idealPartitionBytes) | |
| docsPerPartition := util.Divide(docsCount, idealNumPartitions) | |
| idealNumPartitions := min(1, util.Divide(collBytes, idealPartitionBytes)) | |
| docsPerPartition := min(1, util.Divide(docsCount, idealNumPartitions)) |
There was a problem hiding this comment.
util.Divide() returns a float, so that shouldn’t happen, but I’ve tweaked the logic anyhow.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
$sample-based partitioning has proven problematic for some years now because it often creates highly-imbalanced partitions.
This changeset switches partitioning to use $sampleRate instead. Because this entails a full index scan it tends to be slower; we offset that by creating partition tasks immediately as we receive sampled partition boundaries rather than all at once at the end of the aggregation.
Because MongoDB 4.2 lacked $sampleRate (and $rand as well), the legacy partitioning logic remains for use with that server version.
Both legacy & $sampleRate partitioning are made to use
availableread concern andsecondaryPreferredread preference. These aggregations don’t need consistency, but they benefit substantially from speed & minimizing workload on the primary.A few simplifications are made here as well. For example,
MongosyncIDis removed from the PartitionKey struct since it’s never actually relevant, and certain parameters to the legacy partitioner are made constant (since they were always used thus). Theutil.Dividefunction is renamedutil.DivideToF64to clarify that its return is a float.