optimize: enhance close() logic of discovery module #7375
Conversation
# Conflicts: # changes/en-us/2.x.md # changes/zh-cn/2.x.md
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #7375 +/- ##
============================================
+ Coverage 58.62% 58.69% +0.07%
- Complexity 571 575 +4
============================================
Files 1269 1269
Lines 45730 45774 +44
Branches 5548 5555 +7
============================================
+ Hits 26807 26867 +60
+ Misses 16344 16312 -32
- Partials 2579 2595 +16
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Personally, I think tests should ideally not depend on execution order.
Would you mind sharing the reason why you chose to specify the order in this case? 🤔
There was a problem hiding this comment.
RegistryService is a single instance mode. When testing the whole life cycle, it is generally required to follow the order of register-> lookup (subscribe) -> unregister-> close. Otherwise, if the test method is executed concurrently or the order is not fixed, the required resources will be destroyed.
There was a problem hiding this comment.
Or is there a better way to do it, and I do see lower efficiency in the test than in the normal test
There was a problem hiding this comment.
Thanks for the explanation!
Since it’s implemented as a singleton and shares internal static fields, it does make sense that tests could affect each other.
While it might be possible to make each test fully isolated, I agree that using @Order seems like a more practical solution in this case.
I don’t think any changes are needed in this case!
|
I’ll take a look at the more urgent PRs first, and then come back to review this one! Let’s keep it up and make GSoC a success together! 🙌 |
Ok.😄 |
Ⅰ. Describe what this PR did
close()method.close()as best practice, If the client's lifecycle is controlled by the underlying layer, keep the original logic.RegistryHeartBeats.close()is only called in discovery-redis module, adding the logic to shutdownRegistryHeartBeatsfor discovery-consul module and discovery-etcd module(useRegistryHeartBeatsin the same way).Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews