chore: limit queries to ds #2446
Conversation
BonapartePC
force-pushed
the
chore.limitDSQuery
branch
from
34ce6ab
to
2b690e8
Compare
Codecov ReportBase: 38.56% // Head: 38.67% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2446 +/- ##
==========================================
+ Coverage 38.56% 38.67% +0.11%
==========================================
Files 166 166
Lines 36636 36674 +38
==========================================
+ Hits 14127 14182 +55
+ Misses 21595 21576 -19
- Partials 914 916 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
BonapartePC
force-pushed
the
chore.limitDSQuery
branch
from
947650e
to
81306fd
Compare
| for _, ds := range dsList { | ||
| unprocessedJobs, err := jd.getUnprocessedJobsDS(ctx, ds, true, params) | ||
| if dsLimit > 0 && dsQueryCount >= dsLimit { |
There was a problem hiding this comment.
it would be useful to have another counter stat here to know how often we are reaching limits our
There was a problem hiding this comment.
What is the default max dsLimit?
|
@BonapartePC let's add a test verifying this new jobsDB configuration option |
BonapartePC
force-pushed
the
chore.limitDSQuery
branch
from
7350900
to
9b13f5f
Compare
| return res.Jobs | ||
| } | ||
|
|
||
| t.Run("Test cache with ds limit as one", func(t *testing.T) { |
BonapartePC
force-pushed
the
chore.limitDSQuery
branch
from
bb118cb
to
4cf6599
Compare
BonapartePC
force-pushed
the
chore.limitDSQuery
branch
from
4cf6599
to
3647c0e
Compare
| dsQueryCount := 0 | ||
| var dsLimit int |
There was a problem hiding this comment.
[minor] for consistency and staying idiomatic
| dsQueryCount := 0 | |
| var dsLimit int | |
| var dsQueryCount int | |
| var dsLimit int |
| if dsHit { | ||
| dsQueryCount++ | ||
| } |
There was a problem hiding this comment.
We only increase dsQueryCount when a dsHit takes place, and the decision to stop is based on that.
What if dsQueryCount:
- was measured regardless of queries hit ?
- we stopped early once the dsQueryCount was reached and we had non-zero data.
Letting
The current approach makes sense, but I feel it doesn't penalise queries early enough when we have sparse entries.
Assume we have 200 tables and data are only on in table 3:
- first one
- one in the middle
- one in the end
Now, this algorithm will scan all 200 tables twice until the cache kicks in and avoid the lookup in some tables.
With the approaches I am suggesting, it will require 5 queries for the tables to be scanned, but those queries will be bounded.
There was a problem hiding this comment.
@lvrach a dsHit happens whenever we query the dataset, regardless if the query returns results or not.
The only occasions where dsHit will be returned as false are:
- If we have a cache hit (no jobs & no query)
- An error is returned for whatever reason
There was a problem hiding this comment.
Got it, my bad it wasn't clear from the diff.
A small comment about what getProcessedJobsDS returns would be useful here.
Description
Two changes are made in this PR:
Notion Ticket
https://www.notion.so/rudderstacks/Add-stats-for-the-number-of-DS-per-query-41c957dab7eb414397cd7653a7c65701
https://www.notion.so/rudderstacks/Limit-the-number-of-DS-queries-per-jobsdb-Get-4db41289310940a8b49ae2af7b36f6f3
Security