Implement endpoint watch and resolver #12669
Conversation
| for _, kv := range resp.Kvs { | ||
| var iup internal.Update | ||
| if err := json.Unmarshal(kv.Value, &iup); err != nil { | ||
| continue |
There was a problem hiding this comment.
Maybe worth logging the error at least (as WARNING with the 'corrupted' key)
There was a problem hiding this comment.
Should the logger(*zap.Logger) be created internally or passed from outside?
There was a problem hiding this comment.
Good question.
- I assumed clientv3.lg, but this one is private.
- logger.go is practically dead - I will prepare PR to delete this one.
So there are 2 options:
- Expose the public c.Logger() getter and use it.
- Expand signature of NewManager to take the logger parameter.
There was a problem hiding this comment.
OK. Let's go for 1. It's in scope of the same 'library' and it's hard to imagine use-case when user want's to have separate logger for endpoint resolution.
Please comment that this method is for internal use of etcd-client library and should not be used as general-purpose logger.
| if err == nil { | ||
| up := &Update{Op: op, Key: string(e.Kv.Key), Endpoint: Endpoint{Addr: iup.Addr, Metadata: iup.Metadata}} | ||
| deltaUps = append(deltaUps, up) | ||
| } |
| for _, kv := range resp.Kvs { | ||
| var iup internal.Update | ||
| if err := json.Unmarshal(kv.Value, &iup); err != nil { | ||
| continue |
There was a problem hiding this comment.
Good question.
- I assumed clientv3.lg, but this one is private.
- logger.go is practically dead - I will prepare PR to delete this one.
So there are 2 options:
- Expose the public c.Logger() getter and use it.
- Expand signature of NewManager to take the logger parameter.
4cd5b27 to
b07886c
Compare
| @@ -110,7 +110,6 @@ func TestEtcdGrpcResolver(t *testing.T) { | |||
| } | |||
| t.Fatalf("unexpected response from foo (e2): %s", resp.GetPayload().GetBody()) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
FYI:
git commit -amend
git push --force
Is usual "hack" to re-trigger tests.
05812c2 to
a542896
Compare
8bd48ed to
8feb55f
Compare
|
Thank you |
…opy change Signed-off-by: Nate W <4453979+nate-double-u@users.noreply.github.com>
| @@ -112,7 +113,7 @@ func New(cfg Config) (*Client, error) { | |||
| // service interface implementations and do not need connection management. | |||
| func NewCtxClient(ctx context.Context) *Client { | |||
| cctx, cancel := context.WithCancel(ctx) | |||
| return &Client{ctx: cctx, cancel: cancel, lg: zap.NewNop()} | |||
| return &Client{ctx: cctx, cancel: cancel, lgMu: new(sync.RWMutex), lg: zap.NewNop()} | |||
There was a problem hiding this comment.
@chaochn47 It seems that you missed this change in the second commit in #16800?
There was a problem hiding this comment.
Thanks for capturing that!
Zero value of RWMutex should be the same new(sync.RWMutext) though.
There was a problem hiding this comment.
Zero value of RWMutex should be the same new(sync.RWMutext) though.
It isn't true. zero value for a pointer is nil, while the result of new(sync.RWMutext) isn't nil.
Fix #12652
@peterbourgon