Skip to content

[AMORO-2714]Delete resource when optimizer instance expired#2715

Closed
Yao-MR wants to merge 1 commit into
apache:masterfrom
Yao-MR:improvement/issue-#2714
Closed

[AMORO-2714]Delete resource when optimizer instance expired#2715
Yao-MR wants to merge 1 commit into
apache:masterfrom
Yao-MR:improvement/issue-#2714

Conversation

@Yao-MR

@Yao-MR Yao-MR commented Apr 3, 2024

Copy link
Copy Markdown

Why are the changes needed?

Close #2714

Brief change log

Delete resource when optimizer instance expired which will cause dirty data in db. and this data will never be used again.

How was this patch tested?

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (yes )
  • If yes, how is the feature documented? (not applicable )

@github-actions github-actions Bot added the module:ams-server Ams server module label Apr 3, 2024
@codecov-commenter

codecov-commenter commented Apr 5, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.01%. Comparing base (0c5a173) to head (34bb106).

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2715      +/-   ##
============================================
- Coverage     34.02%   34.01%   -0.02%     
- Complexity     4363     4364       +1     
============================================
  Files           604      604              
  Lines         50752    50753       +1     
  Branches       6673     6673              
============================================
- Hits          17267    17262       -5     
- Misses        32085    32093       +8     
+ Partials       1400     1398       -2     
Flag Coverage Δ
core 32.27% <100.00%> (-0.02%) ⬇️
trino 50.95% <ø> (+0.02%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.forEach(task -> retryTask(task, queue)));
if (isExpired) {
LOG.info("Optimizer {} has been expired, unregister it", keepingTask.getOptimizer());
deleteResource(keepingTask.getOptimizer().getResourceId());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The createResource() maybe called by user.
Is it proper that system auto deleting resources managed by user himself?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

the createResource is actually called by user,
and the deleteResource is also will be called by user, if user want the release the optimzier,

but if the optimizer instance failed by some unexpected casuse, the system will delete the instance auto, but will
not clean the resource record in the record, and this resource will never be used again and will cause dead dirty data in the db forever.

so when the instance is expired and be deleted by system, the resource associate with this instance should also be cleand

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't quite agree with that.

The record left in db has its meaning, cause when the optimizer came back, AMS could identify this resource.
What you said I think is a dashboard design issue, the optimizer page don't reveal inactive optimizers. I think we could improve in that way.
A good and simple princeple is who creates who deletes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thanks your comment jin, agree with it, and will check the lifetime of resource again ,and get one graceful idea the resolve it.

@zhoujinsong

Copy link
Copy Markdown
Contributor

@Yao-MR Thanks a lot for driving this improvement.

I have some questions about this PR:

  • The resource id of the optimizer may be null, you may want to consider the situation.
  • The optimizer may reconnect to the AMS even if the token is expired, and it will request a new token with the same resource id. So it may be dangerous to delete the resources as soon as the token expires.

@Yao-MR

Yao-MR commented Apr 12, 2024

Copy link
Copy Markdown
Author

consider the pr do not resolve this issue in an graceful way, will review this issue again, and redesign the code

@Yao-MR Yao-MR closed this Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module:ams-server Ams server module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement]: clean the resource when optimizer instance expired

4 participants