Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(scheduler): remove microsecond component from scheduled_time #835

Merged
merged 1 commit into from
Mar 30, 2022

Conversation

Samyak2
Copy link
Contributor

@Samyak2 Samyak2 commented Mar 21, 2022

Fixes #830

Changes

This ensures that scheduled_time is always lesser than last_scheduled_time_rca if the date, hour, minute and second match exactly. This prevents DeepDrills/Anomaly from being scheduled again if the microsecond component happens to be greater when all other components match.

Tested this on 15 KPIs - none of them displayed the bug.

  • Add test cases for this - current time or last scheduled time with non-zero microseconds component

Fixes #830

This ensures that `scheduled_time` is always lesser than
`last_scheduled_time_rca` if the date, hour, minute and second match
exactly. This prevents DeepDrills/Anomaly from being scheduled again if
the microsecond component happens to be greater when all other
components match.

Tested this on 15 KPIs - none of them displayed the bug.

Co-authored-by: Rajdeep Sharma <rjdp9736@gmail.com>
@Samyak2 Samyak2 added 🐛 bug Something isn't working 🛠️ backend labels Mar 21, 2022
@Samyak2 Samyak2 added this to the v0.6.0 milestone Mar 21, 2022
@netlify
Copy link

netlify bot commented Mar 21, 2022

✅ Deploy Preview for frontend-sb canceled.

🔨 Explore the source changes: fe82a5f

🔍 Inspect the deploy log: https://app.netlify.com/sites/frontend-sb/deploys/62386407ee97b8000827cc0e

@Samyak2 Samyak2 added the don't merge PR which doesn't need to be merged right now label Mar 21, 2022
@Samyak2 Samyak2 changed the base branch from main to develop March 21, 2022 12:23
@varunp2k varunp2k self-requested a review March 22, 2022 00:08
@manassolanki
Copy link
Member

manassolanki commented Mar 22, 2022

@Samyak2 Can we include this in v0.5.1?

@Samyak2
Copy link
Contributor Author

Samyak2 commented Mar 22, 2022

Yes, this has been tested thoroughly in an internal instance. Test cases to prevent this issue in the future can be added in a separate PR.

Edit: daily anomaly and DD were not tested. Would be better to include in v0.6.0.

@Samyak2 Samyak2 modified the milestones: v0.6.0, v0.5.1 Mar 22, 2022
@Samyak2 Samyak2 requested a review from rjdp March 30, 2022 05:05
@Samyak2 Samyak2 removed the don't merge PR which doesn't need to be merged right now label Mar 30, 2022
Copy link
Contributor

@rjdp rjdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, based on testing we had done.

@Samyak2
Copy link
Contributor Author

Samyak2 commented Mar 30, 2022

Tests will be updated in a separate PR

@Samyak2 Samyak2 merged commit 28af6f6 into develop Mar 30, 2022
@Samyak2 Samyak2 deleted the fix/scheduler-microseconds branch March 30, 2022 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠️ backend 🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] DeepDrills is scheduled one time extra on the same day
3 participants