[DS-6616][WorkerServer] fix worker stop fail and fakes death#6621
[DS-6616][WorkerServer] fix worker stop fail and fakes death#6621lenboo merged 3 commits intoapache:devfrom
Conversation
| try { | ||
| String address = getLocalAddress(); | ||
| Set<String> workerZkPaths = getWorkerZkPaths(); | ||
| for (String workerZkPath : workerZkPaths) { | ||
| registryClient.remove(workerZkPath); | ||
| logger.info("worker node : {} unRegistry from ZK {}.", address, workerZkPath); | ||
| } | ||
| this.heartBeatExecutor.shutdownNow(); | ||
| logger.info("heartbeat executor shutdown"); | ||
| registryClient.close(); | ||
| } catch (Exception ex) { |
There was a problem hiding this comment.
It might better just catch KeeperException.ConnectionLossException here?
| try { | |
| String address = getLocalAddress(); | |
| Set<String> workerZkPaths = getWorkerZkPaths(); | |
| for (String workerZkPath : workerZkPaths) { | |
| registryClient.remove(workerZkPath); | |
| logger.info("worker node : {} unRegistry from ZK {}.", address, workerZkPath); | |
| } | |
| this.heartBeatExecutor.shutdownNow(); | |
| logger.info("heartbeat executor shutdown"); | |
| registryClient.close(); | |
| } catch (Exception ex) { | |
| try { | |
| String address = getLocalAddress(); | |
| Set<String> workerZkPaths = getWorkerZkPaths(); | |
| for (String workerZkPath : workerZkPaths) { | |
| registryClient.remove(workerZkPath); | |
| logger.info("worker node : {} unRegistry from ZK {}.", address, workerZkPath); | |
| } | |
| } catch (KeeperException.ConnectionLossException ex) { | |
| logger.error("remove worker zk path exception", ex) | |
| } | |
| this.heartBeatExecutor.shutdownNow(); | |
| logger.info("heartbeat executor shutdown"); | |
| registryClient.close(); |
There was a problem hiding this comment.
I think it should catch all exception here, because it is the final catch for remove registry info. If it juse catch some exception, the problem will arise again.
There was a problem hiding this comment.
What I concern is that when the connection lost, will it reconnect again? Since the catch scope here may cause the registry.close() will not be executed.
And other thing need to confirm, will it cause some thread fail to exit? I am not clear.
There was a problem hiding this comment.
Yes, you're right. It should be better to just try catch of remove zk path, exclude heartBeatExecutor and registryClient close.
I have modified it.
Codecov Report
@@ Coverage Diff @@
## dev #6621 +/- ##
============================================
- Coverage 38.51% 38.50% -0.01%
- Complexity 3221 3222 +1
============================================
Files 646 646
Lines 25840 25844 +4
Branches 2799 2799
============================================
- Hits 9952 9951 -1
- Misses 14974 14980 +6
+ Partials 914 913 -1
Continue to review full report at Codecov.
|
|
Kudos, SonarCloud Quality Gate passed! |
* [DS-6616][WorkerServer] fix worker stop fail and fakes death * remove unuse test * just add try catch to remove zk worker path Co-authored-by: caishunfeng <534328519@qq.com>
this pr close #6616
Purpose of the pull request
Brief change log
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows: