Batch database queries in RdePipeline - #3190
Conversation
bba0ba9 to
541e340
Compare
03657b3 to
2995c7f
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
+a:@weiminyu to have a look as well.
@CydeWeys made 1 comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on weiminyu).
CydeWeys
left a comment
There was a problem hiding this comment.
This is going to require some seriously careful testing on sandbox.
@CydeWeys made 1 comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on weiminyu).
weiminyu
left a comment
There was a problem hiding this comment.
@weiminyu reviewed 3 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on gbrodman).
|
OK so I tested this on alpha a ton. On alpha, the base repo actually fails to run at all because a toooooon of domains reference the same host -- so we'd try to load the host a million times. The only thing I added against the base repository when testing the baseline was the "Deduplicate referenced hosts for grouping" step, line 463 here. This doesn't really appear to run any more quickly, at least on alpha. This could be because of the skew in alpha data, but more likely it's because batching means that we need to wait until a batch is ready before sending it off to the next step, versus in the old system we could just send each domain/host off immediately to the next step when it was done. This is still worth doing though, because it'll decrease the load on the database massively. #3206 reduces the time of the initial query by a large chunk too, from like two minutes down to 30 seconds. The RDE output for a given day is the same in the old version and this new version |
|
going to make a few more changes + add more tests, no need to review yet |
Currently, we process (repoId, revisionId) pairs for DomainHistory and HostHistory individually -- they may be farmed out to worker nodes in parallel, but each EppResource uses a separate transaction and a separate read, which doesn't scale well when there are lots of domains/hosts. So as a result, we should batch them up so we can load (by default) 500 per transaction at a time. We don't want to batch-load the domains/hosts at the same time that we retrieve the most recent history entry for each type -- this would mean passing relatively large objects across pipeline steps. Instead, we keep passing the KV<String, Long> and batch retrievals. This isn't necessarily much faster (due to having to wait on batching) but there'll be less load on the DB. Self-scan D.2 number 5
|
OK PTAL -- this looks like a lot of code but the main things it actually changes are
|
weiminyu
left a comment
There was a problem hiding this comment.
@weiminyu reviewed 4 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on gbrodman).
Currently, we process (repoId, revisionId) pairs for DomainHistory and HostHistory individually -- they may be farmed out to worker nodes in parallel, but each EppResource uses a separate transaction and a separate read, which doesn't scale well when there are lots of domains/hosts. So as a result, we should batch them up so we can load (by default) 500 per transaction at a time.
We don't want to batch-load the domains/hosts at the same time that we retrieve the most recent history entry for each type -- this would mean passing relatively large objects across pipeline steps. Instead, we keep passing the KV<String, Long> and batch retrievals.
Self-scan D.2 number 5
This change is