feat: add account_advisory backfill job - #2285
Open
katarinazaprazna wants to merge 1 commit into
Open
Conversation
Reviewer's GuideAdds a new one-off account advisory backfill job, wires it into the job runner and Kubernetes cronjob configuration, and introduces supporting cache-task orchestration and public advisory drift checking to validate the populated data. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
katarinazaprazna
marked this pull request as ready for review
July 29, 2026 23:38
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The backfill goroutines don’t appear to honor any cancellation/timeout context, so consider threading a context through
BackfillAccountAdvisory/WithTxto allow the job to terminate promptly when the process receives a shutdown signal. - The fixed concurrency limit (
guard := make(chan struct{}, 4)) is hard-coded; consider making this configurable (e.g., via an environment variable) so you can tune throughput vs. DB load without code changes. - Per-account
LogInfocalls in the inner loop may generate very noisy logs for largerh_accounttables; you might want to log only on failures and periodically on progress (e.g., every N accounts) instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The backfill goroutines don’t appear to honor any cancellation/timeout context, so consider threading a context through `BackfillAccountAdvisory`/`WithTx` to allow the job to terminate promptly when the process receives a shutdown signal.
- The fixed concurrency limit (`guard := make(chan struct{}, 4)`) is hard-coded; consider making this configurable (e.g., via an environment variable) so you can tune throughput vs. DB load without code changes.
- Per-account `LogInfo` calls in the inner loop may generate very noisy logs for large `rh_account` tables; you might want to log only on failures and periodically on progress (e.g., every N accounts) instead.
## Individual Comments
### Comment 1
<location path="tasks/caches/backfill_account_advisory.go" line_range="31" />
<code_context>
+
+ utils.LogInfo("accounts", len(rhAccountIDs), "starting account_advisory backfill")
+
+ guard := make(chan struct{}, 4)
+
+ for i, rhAccountID := range rhAccountIDs {
</code_context>
<issue_to_address>
**suggestion:** Make the backfill concurrency level configurable instead of hard-coded.
The guard channel currently hard-codes concurrency to 4, which may not suit all environments or align with other job settings. Please source this from configuration (or at least a shared constant) so it can be tuned without code changes and kept consistent with other concurrency controls.
Suggested implementation:
```golang
utils.LogInfo("accounts", len(rhAccountIDs), "starting account_advisory backfill")
guard := make(chan struct{}, accountAdvisoryBackfillConcurrency)
```
Please also:
1. Define a shared constant near the top of `tasks/caches/backfill_account_advisory.go` (or in a common config/constants file) like:
`const accountAdvisoryBackfillConcurrency = 4`
2. If your codebase has a central configuration mechanism (e.g., environment-driven or a config struct), consider wiring `accountAdvisoryBackfillConcurrency` from there instead of a literal, so this value can be tuned per environment.
</issue_to_address>
### Comment 2
<location path="tasks/caches/backfill_account_advisory.go" line_range="19-22" />
<code_context>
+
+func backfillAccountAdvisoryPerAccounts(wg *sync.WaitGroup) {
+ var rhAccountIDs []int
+ err := tasks.WithReadReplicaTx(func(tx *gorm.DB) error {
+ return tx.Table("rh_account").
+ Order("hash_partition_id(id, 128), id").
+ Pluck("id", &rhAccountIDs).Error
+ })
+ if err != nil {
</code_context>
<issue_to_address>
**suggestion (performance):** Loading all account IDs into memory at once may not scale well for large datasets.
This loads all rh_account IDs into a slice before starting any work, which can use a lot of memory and delay processing if the table is large. Consider batching (e.g., LIMIT/OFFSET, cursor/streaming, or server-side pagination) so the backfill can run incrementally with bounded memory.
Suggested implementation:
```golang
func backfillAccountAdvisoryPerAccounts(wg *sync.WaitGroup) {
const batchSize = 1000
guard := make(chan struct{}, 4)
offset := 0
for {
var rhAccountIDs []int
err := tasks.WithReadReplicaTx(func(tx *gorm.DB) error {
return tx.Table("rh_account").
Order("hash_partition_id(id, 128), id").
Limit(batchSize).
Offset(offset).
Pluck("id", &rhAccountIDs).Error
})
if err != nil {
utils.LogError("err", err, "unable to load rh_account IDs for account_advisory backfill")
return
}
if len(rhAccountIDs) == 0 {
break
}
utils.LogInfo(
"batch_size", len(rhAccountIDs),
"offset", offset,
"starting account_advisory backfill batch",
)
for i, rhAccountID := range rhAccountIDs {
guard <- struct{}{}
```
Because only part of the function body is visible, you need to:
1. Ensure that the existing per-account processing logic (the code that currently follows `guard <- struct{}{}` and uses `i` and `rhAccountID`) remains unchanged and now operates inside the per-batch inner loop.
2. Confirm that the loop and function are properly closed after your existing per-account logic.
3. Optionally, increment `offset` by the number of IDs processed in each batch (e.g., `offset += len(rhAccountIDs)` at the end of the `for { ... }` loop) if you want deterministic pagination; if you rely on a stable order and no concurrent inserts/deletes, you may instead switch to keyset pagination on `id` for better robustness.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2285 +/- ##
==========================================
- Coverage 59.27% 58.95% -0.32%
==========================================
Files 148 149 +1
Lines 9488 9536 +48
==========================================
- Hits 5624 5622 -2
- Misses 3276 3326 +50
Partials 588 588
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
katarinazaprazna
force-pushed
the
add-aggregator-backfill-job
branch
from
July 30, 2026 00:17
b87ee57 to
3fb1216
Compare
katarinazaprazna
marked this pull request as draft
July 30, 2026 00:19
katarinazaprazna
force-pushed
the
add-aggregator-backfill-job
branch
from
July 30, 2026 08:16
3fb1216 to
a8195f1
Compare
katarinazaprazna
force-pushed
the
add-aggregator-backfill-job
branch
from
July 30, 2026 15:21
a8195f1 to
994fe9b
Compare
katarinazaprazna
marked this pull request as ready for review
July 31, 2026 12:27
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The backfill goroutine concurrency is hard-coded to 4; consider making this configurable (e.g., via environment or job config) so you can tune load on the database without code changes.
- The account_advisory backfill goroutines do not appear to honor context cancellation beyond the initial setup; consider wiring context into the per-account work so the job can be aborted cleanly if requested.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The backfill goroutine concurrency is hard-coded to 4; consider making this configurable (e.g., via environment or job config) so you can tune load on the database without code changes.
- The account_advisory backfill goroutines do not appear to honor context cancellation beyond the initial setup; consider wiring context into the per-account work so the job can be aborted cleanly if requested.
## Individual Comments
### Comment 1
<location path="tasks/caches/backfill_account_advisory.go" line_range="38-47" />
<code_context>
+ go func(i, rhAccountID int) {
</code_context>
<issue_to_address>
**suggestion (performance):** Per-account logging at info level in a tight loop may cause excessive log volume.
Per-account info-level logs before/after backfill and on drift check errors will scale with the number of accounts and can significantly increase log volume and overhead. Please consider lowering verbosity for per-account messages (e.g., debug or aggregated progress logs), while keeping actual error logs at info/error.
Suggested implementation:
```golang
err := tasks.WithTx(func(tx *gorm.DB) error {
utils.LogDebug("i", i, "rh_account_id", rhAccountID, "backfilling account_advisory")
return tx.Exec("SELECT backfill_account_advisory(?)", rhAccountID).Error
})
```
To fully implement the logging-verbosity suggestion, you should also:
1. Scan this file for other per-account `utils.LogInfo` calls (e.g., per-account "completed backfill" or drift-check logs) and down-level them to `LogDebug` or aggregate them into periodic summary logs.
2. Ensure that only actual errors (e.g., failed backfill, drift check failures) are logged with `LogInfo`/`LogError`, keeping the high-level batch start/end logs at info level as they are.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+38
to
+47
| go func(i, rhAccountID int) { | ||
| defer func() { | ||
| <-guard | ||
| wg.Done() | ||
| }() | ||
|
|
||
| err := tasks.WithTx(func(tx *gorm.DB) error { | ||
| utils.LogInfo("i", i, "rh_account_id", rhAccountID, "backfilling account_advisory") | ||
| return tx.Exec("SELECT backfill_account_advisory(?)", rhAccountID).Error | ||
| }) |
There was a problem hiding this comment.
suggestion (performance): Per-account logging at info level in a tight loop may cause excessive log volume.
Per-account info-level logs before/after backfill and on drift check errors will scale with the number of accounts and can significantly increase log volume and overhead. Please consider lowering verbosity for per-account messages (e.g., debug or aggregated progress logs), while keeping actual error logs at info/error.
Suggested implementation:
err := tasks.WithTx(func(tx *gorm.DB) error {
utils.LogDebug("i", i, "rh_account_id", rhAccountID, "backfilling account_advisory")
return tx.Exec("SELECT backfill_account_advisory(?)", rhAccountID).Error
})To fully implement the logging-verbosity suggestion, you should also:
- Scan this file for other per-account
utils.LogInfocalls (e.g., per-account "completed backfill" or drift-check logs) and down-level them toLogDebugor aggregate them into periodic summary logs. - Ensure that only actual errors (e.g., failed backfill, drift check failures) are logged with
LogInfo/LogError, keeping the high-level batch start/end logs at info level as they are.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Adds
account_advisory_backfilljob to populate historical data inaccount_advisoryFollow-up
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Add a suspended, schedulable job to backfill historical data into the account_advisory table and perform advisory drift checks per account.
New Features:
Enhancements:
Tests: