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

Optimize zk ListenServiceEvent listener #343

Merged
merged 7 commits into from
Jan 29, 2020
Merged

Conversation

hxmhlt
Copy link
Contributor

@hxmhlt hxmhlt commented Jan 26, 2020

What this PR does:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@codecov-io
Copy link

codecov-io commented Jan 26, 2020

Codecov Report

Merging #343 into develop will increase coverage by 0.21%.
The diff coverage is 62.1%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #343      +/-   ##
===========================================
+ Coverage    66.14%   66.35%   +0.21%     
===========================================
  Files           96      123      +27     
  Lines         6462     7596    +1134     
===========================================
+ Hits          4274     5040     +766     
- Misses        1742     2067     +325     
- Partials       446      489      +43
Impacted Files Coverage Δ
config/generic_service.go 0% <ø> (ø) ⬆️
cluster/directory/static_directory.go 90.47% <ø> (ø) ⬆️
cluster/cluster_impl/failfast_cluster.go 100% <ø> (ø) ⬆️
cluster/loadbalance/random.go 100% <ø> (ø) ⬆️
cluster/cluster_impl/registry_aware_cluster.go 100% <ø> (ø) ⬆️
cluster/cluster_impl/failover_cluster.go 100% <ø> (ø) ⬆️
common/logger/logging.go 100% <ø> (ø) ⬆️
cluster/cluster_impl/broadcast_cluster.go 100% <ø> (ø) ⬆️
config/mock_rpcservice.go 33.33% <ø> (ø) ⬆️
config/config_utils.go 100% <ø> (ø) ⬆️
... and 140 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6809d8d...0d5e447. Read the comment docs.

@@ -513,7 +514,7 @@ func (z *ZookeeperClient) GetChildrenW(path string) ([]string, <-chan zk.Event,
return nil, nil, perrors.Errorf("path{%s} has none children", path)
}
if len(children) == 0 {
return nil, nil, perrors.Errorf("path{%s} has none children", path)
return nil, nil, errNilChildren
Copy link
Member

Choose a reason for hiding this comment

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

i think you should print the zkpath as before, otherwise users don't know what is going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zouyx , @hxmhlt used the errNilChildren later. so there is no need to add the zkPath in the error. maybe an error log is better.

Copy link
Member

Choose a reason for hiding this comment

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

@@ -544,7 +545,7 @@ func (z *ZookeeperClient) GetChildren(path string) ([]string, error) {
return nil, perrors.Errorf("path{%s} has none children", path)
}
if len(children) == 0 {
return nil, perrors.Errorf("path{%s} has none children", path)
return nil, errNilChildren
Copy link
Member

Choose a reason for hiding this comment

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

as above

remoting/zookeeper/listener.go Outdated Show resolved Hide resolved
@AlexStocks
Copy link
Contributor

LGTM

@AlexStocks AlexStocks merged commit 4de49ef into apache:develop Jan 29, 2020
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