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

Historic performance collection when last runtime not set #765

Merged

Conversation

sushanthakumar
Copy link
Collaborator

@sushanthakumar sushanthakumar commented Oct 19, 2021

What this PR does / why we need it:
Issue description:
When new storage is added, performance collection is scheduled after adding job to scheduler with last run time as None.
It is possible that just before the collection cycle is triggered, task process can restart.
During restart, history collection is performed only if last run time is set. In this case, since restart happened before the completion of first collection, last run time is still not set. so we will end up not collecting the first part of performance metric.

Proposed solution:
During the task process restart, if last runtime is not set but job id is already generated, one historic collection can be done for the storage considering its collection interval time

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
Test scenarios verified:

task process restart after storage is just registered but collection not yet happened
task process restart after storage is registered and last run time already set
task process restart after storage is just registered but collection not yet happened (interval < max_history_collection_window)
task process restart after storage is just registered but collection not yet happened (interval > max_history_collection_window)

Release note:

@sushanthakumar sushanthakumar changed the title History collection when last runtime not set Historic performance collection when last runtime not set Oct 19, 2021
@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #765 (d49a0ec) into master (765df24) will decrease coverage by 0.03%.
The diff coverage is 22.22%.

@@            Coverage Diff             @@
##           master     #765      +/-   ##
==========================================
- Coverage   70.43%   70.39%   -0.04%     
==========================================
  Files         163      163              
  Lines       15855    15867      +12     
  Branches     1961     1962       +1     
==========================================
+ Hits        11167    11170       +3     
- Misses       4014     4021       +7     
- Partials      674      676       +2     
Impacted Files Coverage Δ
...ager/scheduler/schedulers/telemetry/job_handler.py 68.38% <22.22%> (-2.95%) ⬇️
delfin/drivers/fake_storage/__init__.py 95.06% <0.00%> (-0.28%) ⬇️

@sushanthakumar
Copy link
Collaborator Author

@NajmudheenCT @joseph-v @ThisIsClark Pls help to review this PR

self.task_id)
try:
telemetry = PerformanceCollectionTask()
telemetry.collect(self.ctx, self.storage_id, self.args,
Copy link
Member

Choose a reason for hiding this comment

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

return status can be captured and logged !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

joseph-v
joseph-v previously approved these changes Oct 20, 2021
Copy link
Collaborator

@joseph-v joseph-v left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@NajmudheenCT NajmudheenCT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@ThisIsClark ThisIsClark left a comment

Choose a reason for hiding this comment

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

LGTM

@ThisIsClark ThisIsClark merged commit 9e39a6b into sodafoundation:master Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants