-
Notifications
You must be signed in to change notification settings - Fork 83
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
SERVER-81279 Create Genny Workloads for UpdateMany #1100
base: master
Are you sure you want to change the base?
Conversation
This passed through my inbox, but is not assigned to me. So two quick thoughts: Also, could you add Keywords along with description and owner? |
@dalyd Sorry for the mixup! I read somewhere to add performance infrastructure for genny PRs, but in retrospect I suppose that means core genny code and not genny workloads. 😅 I can add keywords to the workloads as well. Thanks for taking a glance. |
Sorry about the confusion @brettnawrocki, I've got an eye on using DEVPROD-3142 to simplify the "who should review this" question with a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't started looking at the actual workloads yet but I'm concerned about everything in the templates directory.
Do we really want this in the repo? And if we are going to add template generation to genny I don't think that the patch
command is how it should be implemented.
And I should add for full transparancy that I added some templating support in the past here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some general comments to the yml.base file but overall the workload looks ok.
Do you have any idea how noisy the new workload is? We would generally run a new workload 3 to 5 times to determine how noisy it is.
Owner: "@mongodb/sharding" | ||
Description: | | ||
This workload does the following: | ||
Create an unsharded collection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: either Create a sharded collection with a hashed monotonic int64 shard key.
OR Insert 50k documents of around 20kB each using the monotonic loader.
.
The monotonic incrementing nature of _id
is important w.r.t to the update Filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
SchemaVersion: 2018-07-01 | ||
Owner: "@mongodb/sharding" | ||
Description: | | ||
This workload does the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could we move this point up to the top or change the intro to:
This workload does the following (with PauseMigrationsDuringMultiUpdates enabled):
(and update the relevant patches so that the comment is correct for the other versions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/workloads/sharding/templates/multi_updates/MultiUpdates.yml.base
Outdated
Show resolved
Hide resolved
src/workloads/sharding/templates/multi_updates/MultiUpdates.yml.base
Outdated
Show resolved
Hide resolved
Collection: &Collection coll | ||
Namespace: &Namespace test.coll | ||
|
||
ApproxDocumentSize: &ApproxDocumentSize 20_000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think just DocumentSize is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Operations: | ||
- OperationName: updateMany | ||
OperationCommand: | ||
Filter: { _id: 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: could we half the number of workloads by executing the Filter: {}
and Filter: { _id: 0 }
in every workload? Essentially add a new phase for Filter: { _id: 0 }
to every one.
Although I do appreciate that you are doing exactly what the ticket asks for.
Operations: | ||
- OperationName: updateMany | ||
OperationCommand: | ||
Filter: { _id: 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: you could we half the number of workloads by executing the Filter: {}
and Filter: { _id: 0 }
in every workload? Essentially add a new phase for Filter: { _id: 0 }
to every one.
Although I do appreciate that you are doing exactly what the ticket asks for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment to highlight the differences for each version?
E.g: Filter: { _id: 0 } # updateMany on a single document.
and Filter: { } # updateMany on all documents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the first suggestion and did both kinds of updateManys in separate phases per workload. It does make sense that it's better to reuse the setup if we can for both variants. I didn't add the comments since I think it's clear enough now the purpose of each filter based on the name of the metrics.
I expected this to be somewhat controversial, though as you mention there is some existing precedent for "bring your own code generation" for other workloads in the repo. I see DEVPROD-3567 was filed partially in reaction to this PR. I agree that using My hope is that a better alternative can be proposed (and implemented if necessary), but that this can be used in the meantime.
The cluster parameter in this workload isn't implemented yet (so enabling it does literally nothing). From the first patch, each workload is run effectively twice and I saw ~5% difference between them. I planned to run another patch and then rerun the workload a few times to get better numbers, but my patch failed I believe due to BF-31402. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a suggestion about parameterization rather than code generation.
@@ -15,7 +26,7 @@ GlobalDefaults: | |||
Collection: &Collection coll | |||
Namespace: &Namespace test.coll | |||
|
|||
ApproxDocumentSize: &ApproxDocumentSize 20_000 | |||
DocumentSize: &DocumentSize 20_000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to parameterized this workload and get rid of the code generation?
Some loadconfig examples of this are:
Essentially you would move this file into src/phases/multi_updates and then pass inNops for the pauseMigrationsDuringMultiUpdates and ShardCollection operations as needed. The operations could be parameterized something like the queries in
OperationCommand: {^Parameter: {Name: "OperationCommand", Default: *Q1_1Query}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
New patch build: https://spruce.mongodb.com/version/65a85231a4cf47ebaffecbdd/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC Results on Linux Shard Lite (all feature flags) 2022-11
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the major refactor.
I hope it wasn't too onerous but the workload looks much cleaner now.
updateMany will update all of the 50k documents per command in the first | ||
phase and only a single document per command in the second phase. | ||
|
||
Two parameters are accepted to alter the behavior of this workload: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the documentation!
It was no problem, and as I mentioned before, I expected the code generator to be controversial. 😅 I think the end result is definitely more maintainable than the original iteration. The way |
Hey @brettnawrocki, just taking over from Jim on behalf of the performance team. We’d definitely encourage you to open a DEVPROD ticket (with the Assigned Teams field set to “Performance Infrastructure”) if you think having an “all other phases” feature in Genny would be valuable. Also, we have an OnlyRunInInstance: sharded feature that can be used to conditionally shard a collection (the downside of that is until DEVPROD-2049 is resolved you’ll need to exclude your workloads from dry-runs here). None of this is blocking and I see Jim’s already approved this PR, but just adding this comment in case it adds any value! |
Jira Ticket: SERVER-81279
Whats Changed:
Adds performance workloads for various cases involving updateMany in sharded clusters.
Patch testing results:
https://spruce.mongodb.com/version/6597130356234348f74ef21a/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC