VAULT-14733: Split logic of precomputedQueryWorker #20073
Conversation
There was a problem hiding this comment.
This is another dramatic improvement. Looks great! I threw in some initial superficial review comments regarding the method signatures. I'm going to do another pass on this with more impactful feedback, but just wanted to get the conversation started on a couple of things.
|
|
||
| // pqOptions holds fields that will be used when creating precomputed queries | ||
| // These fields will remain the same for every segment that a precomputed query worker is handling | ||
| type pqOptions struct { |
vault/activity_log.go
Outdated
| // reverse ordering). If yes, we remove those references. This way a | ||
| // client is considered new only in the earliest month of its use in | ||
| // the billing period. | ||
| for currMonth := timeutil.StartOfMonth(segmentTime).UTC(); currMonth != timeutil.StartOfMonth(opts.activePeriodEnd).UTC(); currMonth = timeutil.StartOfNextMonth(currMonth).UTC() { |
There was a problem hiding this comment.
This seems like it could maybe be abstracted out into a rangeMonths method in timeutil, especially if it's used elsewhere (I haven't looked).
There was a problem hiding this comment.
Barring that approach, I think capturing lastMonth := timeutil.StartOfMonth(opts.activePeriodEnd).UTC() outside of this loop and just using that variable in a currMonth.Before(lastMonth) could be useful.
There was a problem hiding this comment.
this is only used in one place, so i didn't make a new method in timeutil. I did clean it up according to your suggestion.
| a.logger.Warn("failed to read segment", "error", err) | ||
| return err | ||
| } | ||
| err = a.handleEntitySegment(entity, segmentTime, hyperloglog, opts) |
There was a problem hiding this comment.
I wish there was a way for us to avoid 3 levels of nested for loops, but I suppose there's a lot going on here, so it's kind of unavoidable.
miagilepner
force-pushed
the
miagilepner/VAULT-14733-split-pq
branch
from
d51cd82
to
0625e2f
Compare
This PR splits up the precomputed query worker so that the logic transforming a segment to a precomputed query sits in its own method. This method will later be used by the client count generation endpoint.