You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "Yao-MR (via GitHub)" <gi...@apache.org> on 2024/04/03 12:36:51 UTC
[PR] [AMORO-2714]Delete resource when optimizer instance expired [incubator-amoro]
Yao-MR opened a new pull request, #2715:
URL: https://github.com/apache/incubator-amoro/pull/2715
## 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?
- [x] 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 )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-2714]Delete resource when optimizer instance expired [incubator-amoro]
Posted by "majin1102 (via GitHub)" <gi...@apache.org>.
majin1102 commented on code in PR #2715:
URL: https://github.com/apache/incubator-amoro/pull/2715#discussion_r1554826050
##########
ams/server/src/main/java/com/netease/arctic/server/DefaultOptimizingService.java:
##########
@@ -548,6 +548,7 @@ public void run() {
.forEach(task -> retryTask(task, queue)));
if (isExpired) {
LOG.info("Optimizer {} has been expired, unregister it", keepingTask.getOptimizer());
+ deleteResource(keepingTask.getOptimizer().getResourceId());
Review Comment:
The createResource() maybe called by user.
Is it proper that system auto deleting resources managed by user himself?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-2714]Delete resource when optimizer instance expired [incubator-amoro]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2715:
URL: https://github.com/apache/incubator-amoro/pull/2715#issuecomment-2038588682
## [Codecov](https://app.codecov.io/gh/apache/incubator-amoro/pull/2715?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 50.95%. Comparing base [(`0c5a173`)](https://app.codecov.io/gh/apache/incubator-amoro/commit/0c5a17368cce5a39292f52d316a20821d64a237e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`34bb106`)](https://app.codecov.io/gh/apache/incubator-amoro/pull/2715?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #2715 +/- ##
=============================================
+ Coverage 34.02% 50.95% +16.93%
+ Complexity 4363 590 -3773
=============================================
Files 604 46 -558
Lines 50752 4708 -46044
Branches 6673 513 -6160
=============================================
- Hits 17267 2399 -14868
+ Misses 32085 2084 -30001
+ Partials 1400 225 -1175
```
| [Flag](https://app.codecov.io/gh/apache/incubator-amoro/pull/2715/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [core](https://app.codecov.io/gh/apache/incubator-amoro/pull/2715/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [trino](https://app.codecov.io/gh/apache/incubator-amoro/pull/2715/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.95% <ø> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-amoro/pull/2715?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-2714]Delete resource when optimizer instance expired [incubator-amoro]
Posted by "Yao-MR (via GitHub)" <gi...@apache.org>.
Yao-MR commented on code in PR #2715:
URL: https://github.com/apache/incubator-amoro/pull/2715#discussion_r1554834104
##########
ams/server/src/main/java/com/netease/arctic/server/DefaultOptimizingService.java:
##########
@@ -548,6 +548,7 @@ public void run() {
.forEach(task -> retryTask(task, queue)));
if (isExpired) {
LOG.info("Optimizer {} has been expired, unregister it", keepingTask.getOptimizer());
+ deleteResource(keepingTask.getOptimizer().getResourceId());
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-2714]Delete resource when optimizer instance expired [amoro]
Posted by "Yao-MR (via GitHub)" <gi...@apache.org>.
Yao-MR commented on PR #2715:
URL: https://github.com/apache/amoro/pull/2715#issuecomment-2051090570
consider the pr do not resolve this issue in an graceful way, will review this issue again, and redesign the code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-2714]Delete resource when optimizer instance expired [amoro]
Posted by "Yao-MR (via GitHub)" <gi...@apache.org>.
Yao-MR commented on code in PR #2715:
URL: https://github.com/apache/amoro/pull/2715#discussion_r1558734343
##########
ams/server/src/main/java/com/netease/arctic/server/DefaultOptimizingService.java:
##########
@@ -548,6 +548,7 @@ public void run() {
.forEach(task -> retryTask(task, queue)));
if (isExpired) {
LOG.info("Optimizer {} has been expired, unregister it", keepingTask.getOptimizer());
+ deleteResource(keepingTask.getOptimizer().getResourceId());
Review Comment:
thanks your comment jin, agree with it, and will check the lifetime of resource again ,and get one graceful idea the resolve it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-2714]Delete resource when optimizer instance expired [incubator-amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on PR #2715:
URL: https://github.com/apache/incubator-amoro/pull/2715#issuecomment-2042108768
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-2714]Delete resource when optimizer instance expired [amoro]
Posted by "majin1102 (via GitHub)" <gi...@apache.org>.
majin1102 commented on code in PR #2715:
URL: https://github.com/apache/amoro/pull/2715#discussion_r1558036625
##########
ams/server/src/main/java/com/netease/arctic/server/DefaultOptimizingService.java:
##########
@@ -548,6 +548,7 @@ public void run() {
.forEach(task -> retryTask(task, queue)));
if (isExpired) {
LOG.info("Optimizer {} has been expired, unregister it", keepingTask.getOptimizer());
+ deleteResource(keepingTask.getOptimizer().getResourceId());
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-2714]Delete resource when optimizer instance expired [amoro]
Posted by "Yao-MR (via GitHub)" <gi...@apache.org>.
Yao-MR closed pull request #2715: [AMORO-2714]Delete resource when optimizer instance expired
URL: https://github.com/apache/amoro/pull/2715
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org