feat: add property-based job disable gate in runner#1897
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 57 minutes and 2 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Benchstat (Other)Base: 📊 2 minor regression(s) (all within 5% threshold)
Full benchstat output |
Benchstat (RLS)Base: 📊 4 minor regression(s) (all within 5% threshold)
✅ 4 improvement(s)
Full benchstat output |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
job/job.go (2)
396-404: Disable gate runs afterinit()side effects.
j.init()starts the history evictor, populates the status ring from the DB, and mutates context/object metadata before the disable check. If the intent is to make a disabled job a true no-op, consider evaluating the disable properties prior toj.init()(the property keys don't depend on anything init sets). As-is, a disabled job still incurs DB work and evictor startup on the first call. Not a correctness bug sinceinit()is idempotent viaj.initialized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@job/job.go` around lines 396 - 404, The disable-property check currently runs after j.init(), causing initialization side effects (history evictor, DB reads) even for disabled jobs; move the properties check (the j.Properties().On(false, j.getPropertyNames("disable")...) || j.Properties().On(false, j.getPropertyNames("disabled")...)) to run before calling j.init(), and only call j.init() if the job is not disabled; keep the existing r.Failf("failed to initialize job: %s", r.ID()) handling for init errors and preserve early returns to avoid starting the evictor or touching the DB for disabled jobs.
401-404: Consider collapsing both property lookups into a singleOn()call.
Properties().On()accepts variadic keys and returns on first match, so both alias sets can be combined. This also avoids registering property metadata twice vianewProp(seecontext/properties.go:77-99).♻️ Suggested refactor
- if j.Properties().On(false, j.getPropertyNames("disable")...) || j.Properties().On(false, j.getPropertyNames("disabled")...) { - ctx.Tracef("job disabled via properties") - return - } + disableKeys := append(j.getPropertyNames("disable"), j.getPropertyNames("disabled")...) + if j.Properties().On(false, disableKeys...) { + ctx.Tracef("job disabled via properties") + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@job/job.go` around lines 401 - 404, The two separate property checks should be collapsed into a single call to Properties().On to avoid duplicate metadata registration; replace the two calls to j.Properties().On(false, j.getPropertyNames("disable")...) and j.Properties().On(false, j.getPropertyNames("disabled")...) with one call that passes both key slices (e.g. combine the results of j.getPropertyNames("disable") and j.getPropertyNames("disabled") into a single variadic argument) while keeping the same default false and preserving the ctx.Tracef("job disabled via properties") return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@job/job.go`:
- Around line 396-404: The disable-property check currently runs after j.init(),
causing initialization side effects (history evictor, DB reads) even for
disabled jobs; move the properties check (the j.Properties().On(false,
j.getPropertyNames("disable")...) || j.Properties().On(false,
j.getPropertyNames("disabled")...)) to run before calling j.init(), and only
call j.init() if the job is not disabled; keep the existing r.Failf("failed to
initialize job: %s", r.ID()) handling for init errors and preserve early returns
to avoid starting the evictor or touching the DB for disabled jobs.
- Around line 401-404: The two separate property checks should be collapsed into
a single call to Properties().On to avoid duplicate metadata registration;
replace the two calls to j.Properties().On(false,
j.getPropertyNames("disable")...) and j.Properties().On(false,
j.getPropertyNames("disabled")...) with one call that passes both key slices
(e.g. combine the results of j.getPropertyNames("disable") and
j.getPropertyNames("disabled") into a single variadic argument) while keeping
the same default false and preserving the ctx.Tracef("job disabled via
properties") return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ad28415-d8f1-4fb0-9afd-09dc5ea04114
📒 Files selected for processing (2)
job/job.gotests/job_test.go
adityathebe
force-pushed
the
aditya/job-disable-property
branch
from
1a2f05e to
a1c0fda
Compare
2 participants
Summary by CodeRabbit
Release Notes
New Features
Tests