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

clientv3/naming: ignore empty update. #10582

Merged
merged 1 commit into from Apr 30, 2019

Conversation

5 participants
@johncming
Copy link
Contributor

commented Mar 25, 2019

ignore empty update.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 25, 2019

Codecov Report

Merging #10582 into master will increase coverage by 0.12%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10582      +/-   ##
==========================================
+ Coverage   71.63%   71.75%   +0.12%     
==========================================
  Files         393      393              
  Lines       36568    36570       +2     
==========================================
+ Hits        26195    26242      +47     
+ Misses       8538     8496      -42     
+ Partials     1835     1832       -3
Impacted Files Coverage Δ
clientv3/naming/grpc.go 75.43% <0%> (-2.75%) ⬇️
etcdserver/api/v3rpc/lease.go 71.59% <0%> (-5.69%) ⬇️
clientv3/retry_interceptor.go 68.13% <0%> (-0.99%) ⬇️
etcdserver/api/rafthttp/stream.go 79.39% <0%> (-0.43%) ⬇️
etcdserver/server.go 74.82% <0%> (-0.36%) ⬇️
clientv3/balancer/grpc1.7-health.go 59.59% <0%> (+0.29%) ⬆️
etcdserver/api/v3rpc/watch.go 82.35% <0%> (+0.32%) ⬆️
clientv3/op.go 72.91% <0%> (+0.41%) ⬆️
clientv3/watch.go 92.19% <0%> (+0.42%) ⬆️
etcdserver/api/membership/cluster.go 77.95% <0%> (+0.45%) ⬆️
... and 13 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 7a5acb4...cb39c97. Read the comment docs.

@@ -98,6 +98,8 @@ func (gw *gRPCWatcher) Next() ([]*naming.Update, error) {
case etcd.EventTypeDelete:
err = json.Unmarshal(e.PrevKv.Value, &jupdate)
jupdate.Op = naming.Delete
default:

This comment has been minimized.

Copy link
@spzala

spzala Mar 25, 2019

Member

Looks good and it doesn't harm but curious, did you run into such case? Any empty values should be already ignored by this point. Thanks!

This comment has been minimized.

Copy link
@johncming

johncming Mar 27, 2019

Author Contributor

Yes, it is ok not to add default at present, but it is more consistent with the intention from the logic point of view.

Use this pr if it helps. Thanks.

This comment has been minimized.

Copy link
@spzala

spzala Mar 27, 2019

Member

I wanted to understand the reason behind fix and see if a test is needed. If it's more of refactoring we should update the PR message accordingly. Since empty values are already taken care by this point, default might not be added originally considering it gives impression that there can be empty values. Thank you for all your PRs.

This comment has been minimized.

Copy link
@johncming

johncming Mar 28, 2019

Author Contributor

yes, I should modify the commit message. not "ignore empty update", It should be "ignore unknown update type'

In combination with the situation you mentioned, it is better to use continue to ignore unknown type or panic?

Examples of panic processing are available in the code.

switch ev.Type {
case mvccpb.PUT:
ops = append(ops, clientv3.OpPut(modifyPrefix(string(ev.Kv.Key)), string(ev.Kv.Value)))
atomic.AddInt64(&total, 1)
case mvccpb.DELETE:
ops = append(ops, clientv3.OpDelete(modifyPrefix(string(ev.Kv.Key))))
atomic.AddInt64(&total, 1)
default:
panic("unexpected event type")

thanks!

This comment has been minimized.

Copy link
@spzala

spzala Apr 12, 2019

Member

Thanks! I don't have a strong opinion on it but using continue seems good enough as there isn't any real bug reported related to it. @hexfusion

@jingyih

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

cc @jpbetz

@xiang90

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

lgtm

@xiang90 xiang90 merged commit fc69368 into etcd-io:master Apr 30, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.