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/integration: add TestKVCompact #4413

Merged
merged 1 commit into from
Feb 4, 2016
Merged

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Feb 4, 2016

Finishing #4338.

@heyitsanthony
Copy link
Contributor

lgtm-- should we also test that compact closes a watcher, though?

@gyuho
Copy link
Contributor Author

gyuho commented Feb 4, 2016

Sure. Will add it too. Thanks.

On Thu, Feb 4, 2016, 08:30 Anthony Romano notifications@github.com wrote:

lgtm-- should we also test that compact closes a watcher, though?


Reply to this email directly or view it on GitHub
#4413 (comment).

Sincerely,
Gyu-Ho Lee

@xiang90
Copy link
Contributor

xiang90 commented Feb 4, 2016

lgtm-- should we also test that compact closes a watcher, though?

hmm... This is testing the client actually. So we should not test the server behavior of closing the watchers here. Right?

t.Fatalf("couldn't compact kv space (%v)", err)
}
err := kv.Compact(2)
if err == nil || !strings.Contains(err.Error(), storage.ErrCompacted.Error()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please add these types to v3rpc/errors.go and add the error handling to togRPCError func?

Copy link
Contributor

Choose a reason for hiding this comment

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

then we can do err != v3rpc.ErrCompacted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Will add. Thanks.

On Thu, Feb 4, 2016, 09:17 Xiang Li notifications@github.com wrote:

In clientv3/integration/kv_test.go
#4413 (comment):

  • clus := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 3})
  • defer clus.Terminate(t)
  • kv := clientv3.NewKV(clus.RandClient())
  • for i := 0; i < 3; i++ {
  •   if _, err := kv.Put("foo", "", lease.NoLease); err != nil {
    
  •       t.Fatalf("couldn't put 'foo' (%v)", err)
    
  •   }
    
  • }
  • if err := kv.Compact(2); err != nil {
  •   t.Fatalf("couldn't compact kv space (%v)", err)
    
  • }
  • err := kv.Compact(2)
  • if err == nil || !strings.Contains(err.Error(), storage.ErrCompacted.Error()) {

then we can do err != v3rpc.ErrCompacted.


Reply to this email directly or view it on GitHub
https://github.com/coreos/etcd/pull/4413/files#r51904815.

Sincerely,
Gyu-Ho Lee

@heyitsanthony
Copy link
Contributor

It would test the logic in the client responsible for closing an outdated watcher channel on a compact.

@xiang90
Copy link
Contributor

xiang90 commented Feb 4, 2016

@heyitsanthony OK. You are right.

@gyuho
Copy link
Contributor Author

gyuho commented Feb 4, 2016

/cc @heyitsanthony @xiang90

  1. Checks if watch stream on compacted revision gets canceled
  2. Use v3rpc errors for ErrCompacted

PTAL.

Thanks.

wr, ok := <-wchan
empty := reflect.DeepEqual(emptyResp, wr)
if !empty {
t.Fatalf("expected empty response from closed channel, got %+v", wr)
Copy link
Contributor

Choose a reason for hiding this comment

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

we usually print out what we got first, then print out what we want.

@gyuho
Copy link
Contributor Author

gyuho commented Feb 4, 2016

@xiang90 Just addressed. PTAL. Thanks.

wchan := wc.Watch(context.TODO(), "foo", 3)

wr, ok := <-wchan
isEmpty := reflect.DeepEqual(clientv3.WatchResponse{Header: pb.ResponseHeader{}, Events: nil}, wr)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the empty checking since this ensures by go runtime that when a chan is closed it returns empty response.

@xiang90
Copy link
Contributor

xiang90 commented Feb 4, 2016

LGTM

@gyuho
Copy link
Contributor Author

gyuho commented Feb 4, 2016

Just fixed. Will merge after green lights. Thanks!

@gyuho
Copy link
Contributor Author

gyuho commented Feb 4, 2016

Semaphore is not responding... I ran integration test on Google cloud machine and confirmed that all tests pass.

gyuho added a commit that referenced this pull request Feb 4, 2016
clientv3/integration: add TestKVCompact
@gyuho gyuho merged commit d21ef68 into etcd-io:master Feb 4, 2016
@gyuho gyuho deleted the TestKVCompact branch February 5, 2016 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants