Conversation
RJAK11
requested review from
freshstrangemusic,
jaredlockhart,
mikewilli and
yashikakhurana
as code owners
mikewilli
left a comment
mikewilli
left a comment
There was a problem hiding this comment.
Will this work with our error checks? Right now we inject an error inside the client code if we expect results but the file doesn't exist. I think with this new feature we'd just skip over them.
| migrations.AddField( | ||
| model_name='nimbusexperiment', | ||
| name='results_data_updated_at', | ||
| field=models.DateTimeField(blank=True, null=True, verbose_name='Results Data Last Updated'), |
There was a problem hiding this comment.
What's going to be the behavior the first time we run the new version of the task? Refresh every experiment's results whether it needs them or not?
There was a problem hiding this comment.
Yes I believe so. I wasn't able to think of an approach that would safely prevent this.
Because * the fetch task used experiment status and a fixed time window to decide when to fetch results This commit * adds a field to track when an experiment's results were last loaded * finds the newest modified time among each experiment's statistics, metadata, and errors files * refreshes when that timestamp is newer than the stored one, and saves the new timestamp after loading * still refreshes when an expected results window is missing from the bucket so that missing-results errors are injected Fixes #15737
Because * the fetch task used experiment status and a fixed time window to decide when to fetch results This commit * adds a field to track when an experiment's results were last loaded * finds the newest modified time among each experiment's statistics, metadata, and errors files * refreshes when that timestamp is newer than the stored one, and saves the new timestamp after loading * still refreshes when an expected results window is missing from the bucket so that missing-results errors are injected Fixes #15737
Thank you @mikewilli for the review! You are right, the changes would've skipped the error checks and I didn’t notice that. I added a new function so that the task also re-fetches when we expect a window whose file isn't in the bucket, so now the client should still inject the error. It's bounded by the analysis in the exact way we originally used so a result that never arrives doesn't just keep refetching forever. This might be a messy solution but I couldn’t think of another approach. Let me know what you think! |
2 participants
Because
This commit
Fixes #15737