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

Prevent resetting latest flag of real-time when starting historical analysis #1287

Merged
merged 1 commit into from
Aug 31, 2024

Conversation

kaituo
Copy link
Collaborator

@kaituo kaituo commented Aug 30, 2024

Description

This PR addresses a bug where starting a historical analysis after a real-time analysis on the same detector caused the real-time task’s latest flag to be incorrectly reset to false by the historical run.

The fix ensures that only the latest flags of the same analysis type are reset:

  • Real-time analysis will only reset the latest flag of previous real-time analyses.
  • Historical analysis will only reset the latest flag of previous historical analyses.

Additional Changes:

  • Set a minimum recencyEmphasis value of 2 to align with RCF requirements.
  • Updated imputation logic to execute only when the entity maps to the local node ID in the hash ring, reducing errors when models of the same entity exist on multiple nodes.

Testing:

  • Added an integration test to reproduce the bug and verified the fix.

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 53.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 77.55%. Comparing base (3f0fc8c) to head (065a64b).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...search/ad/transport/ADHCImputeTransportAction.java 30.00% 0 Missing and 7 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1287      +/-   ##
============================================
+ Coverage     71.83%   77.55%   +5.72%     
- Complexity     4898     5432     +534     
============================================
  Files           518      532      +14     
  Lines         22879    23263     +384     
  Branches       2245     2305      +60     
============================================
+ Hits          16434    18041    +1607     
+ Misses         5410     4169    -1241     
- Partials       1035     1053      +18     
Flag Coverage Δ
plugin 77.55% <53.33%> (+5.72%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ain/java/org/opensearch/ad/task/ADTaskManager.java 75.40% <100.00%> (-0.24%) ⬇️
.../opensearch/forecast/task/ForecastTaskManager.java 2.76% <ø> (ø)
...g/opensearch/timeseries/ml/RealTimeInferencer.java 90.14% <ø> (ø)
...n/java/org/opensearch/timeseries/model/Config.java 86.02% <100.00%> (-1.39%) ⬇️
...va/org/opensearch/timeseries/task/TaskManager.java 81.38% <100.00%> (+2.81%) ⬆️
...search/ad/transport/ADHCImputeTransportAction.java 69.76% <30.00%> (ø)

... and 105 files with indirect coverage changes

@kaituo kaituo force-pushed the stateBug branch 2 times, most recently from 2d5aff9 to b6503c3 Compare August 30, 2024 19:25
…g historical analysis

This PR addresses a bug where starting a historical analysis after a real-time analysis on the same detector caused the real-time task’s latest flag to be incorrectly reset to false by the historical run.

The fix ensures that only the latest flags of the same analysis type are reset:
* Real-time analysis will only reset the latest flag of previous real-time analyses.
* Historical analysis will only reset the latest flag of previous historical analyses.

This PR also updated recencyEmphasis to have a minimum value of 2, aligning with RCF requirements.

Testing:
- Added an integration test to reproduce the bug and verified the fix.

Signed-off-by: Kaituo Li <kaituo@amazon.com>
* indicating that the model state was updated in previous intervals.
* - The entity associated with the model state is present.
* - The owning node for real-time processing of the entity, with the same local version, is present in the hash ring.
* - The owning node for real-time processing matches the current local node.
Copy link
Member

Choose a reason for hiding this comment

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

before we could have occasions where two nodes have the same model for the same entity, so two nodes at the same time will find if there is an anomaly on the interval?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes.

Copy link
Member

Choose a reason for hiding this comment

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

would this mean we would write result twice or we handled it later to only be written once?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We would only write it once (not guaranteed) as here we only process imputation on a node the entity is mapped to.

@kaituo kaituo merged commit afd5da9 into opensearch-project:main Aug 31, 2024
19 of 20 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 31, 2024
…g historical analysis (#1287)

This PR addresses a bug where starting a historical analysis after a real-time analysis on the same detector caused the real-time task’s latest flag to be incorrectly reset to false by the historical run.

The fix ensures that only the latest flags of the same analysis type are reset:
* Real-time analysis will only reset the latest flag of previous real-time analyses.
* Historical analysis will only reset the latest flag of previous historical analyses.

This PR also updated recencyEmphasis to have a minimum value of 2, aligning with RCF requirements.

Testing:
- Added an integration test to reproduce the bug and verified the fix.

Signed-off-by: Kaituo Li <kaituo@amazon.com>
(cherry picked from commit afd5da9)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
amitgalitz pushed a commit that referenced this pull request Sep 2, 2024
…g historical analysis (#1287) (#1288)

This PR addresses a bug where starting a historical analysis after a real-time analysis on the same detector caused the real-time task’s latest flag to be incorrectly reset to false by the historical run.

The fix ensures that only the latest flags of the same analysis type are reset:
* Real-time analysis will only reset the latest flag of previous real-time analyses.
* Historical analysis will only reset the latest flag of previous historical analyses.

This PR also updated recencyEmphasis to have a minimum value of 2, aligning with RCF requirements.

Testing:
- Added an integration test to reproduce the bug and verified the fix.


(cherry picked from commit afd5da9)

Signed-off-by: Kaituo Li <kaituo@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants