Data Carousel: Protection against duplicate staging rule submission #631
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds protection against duplicate staging rule submission in the ATLAS Data Carousel watchdog by implementing a timer mechanism that prevents the doStageDCRequests method from running beyond its lock time limit.
Key changes:
- Introduces named constants for lock time limits across all watchdog methods
- Adds timer logic in
doStageDCRequeststo break out of the staging loop before the lock expires - Refactors hardcoded
timeLimitvalues to use centralized configuration constants
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| tmpLog.debug("start") | ||
| try: | ||
| # timer start | ||
| t0 = time.monotonic() |
There was a problem hiding this comment.
The timer t0 is started before acquiring the lock (line 56), but it should start after acquiring the lock (line 59). Currently, if the process waits to acquire the lock, that waiting time is incorrectly counted against the lock time limit, potentially causing the loop to exit prematurely.
The timer should be moved to line 63 (after tmpLog.debug("got watchdog lock")) to accurately measure the time the lock is actually held.
| tmpLog.debug(f"stage request_id={dc_req_spec.request_id} dataset={dc_req_spec.dataset}") | ||
| # check timer | ||
| t1 = time.monotonic() | ||
| if t1 - t0 > max(lock_time_sec - 60, lock_time_sec * 0.8): |
There was a problem hiding this comment.
The condition uses max(lock_time_sec - 60, lock_time_sec * 0.8) which will always evaluate to lock_time_sec - 60 when lock_time_sec >= 300 (5 minutes * 60 seconds). This makes the 0.8 factor ineffective.
For the current value of 5 minutes (300 seconds):
lock_time_sec - 60= 240 secondslock_time_sec * 0.8= 240 seconds
They happen to be equal, but for any value >= 300, the subtraction will always be larger. Consider using min(lock_time_sec - 60, lock_time_sec * 0.8) instead to ensure you use the more conservative threshold.
| if t1 - t0 > max(lock_time_sec - 60, lock_time_sec * 0.8): | |
| if t1 - t0 > min(lock_time_sec - 60, lock_time_sec * 0.8): |
No description provided.