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: translate Snapshot API gRPC status error #9038

Merged
merged 2 commits into from
Dec 19, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Dec 18, 2017

To be consistent with other APIs.

Related discussion #9024 (comment).

clus := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 1})
defer clus.Terminate(t)

clus.Members[0].Stop(t)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is needed to write data cluster. this tests should care about about context canceled and deadline exceeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need test canceling the inflight snapshot stream. Without data, it's hard to simulate concurrent context cancel while snapshot is in progress. This manual mvcc writes give us about >1 second window where we can inject concurrent context cancel.

b.Close()
clus.Members[0].Restart(t)

f, err := ioutil.TempFile("", "snapshot.db")
Copy link
Member

Choose a reason for hiding this comment

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

we can use ioutil.Discard instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Will change.

if err != nil && err != context.DeadlineExceeded {
t.Errorf("expected %v, got %v", context.DeadlineExceeded, err)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I was able to shorten the test with suggestions above to the following:

func TestMaintenanceSnapshotError(t *testing.T) {
	defer testutil.AfterTest(t)

	clus := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 1})
	defer clus.Terminate(t)

	// reading snapshot with canceled context should error out
	ctx, cancel := context.WithCancel(context.Background())
	rc1, err := clus.RandClient().Snapshot(ctx)
	if err != nil {
		t.Fatal(err)
	}
	defer rc1.Close()

	cancel()
	_, err = io.Copy(ioutil.Discard, rc1)
	if err != context.Canceled {
		t.Errorf("expected %v, got %v", context.Canceled, err)
	}

	// reading snapshot with deadline exceeded should error out
	ctx, cancel = context.WithTimeout(context.Background(), 100*time.Millisecond)
	defer cancel()
	rc2, err := clus.RandClient().Snapshot(ctx)
	if err != nil {
		t.Fatal(err)
	}
	defer rc2.Close()

	time.Sleep(200 * time.Millisecond)

	_, err = io.Copy(ioutil.Discard, rc2)
	if err != nil && err != context.DeadlineExceeded {
		t.Errorf("expected %v, got %v", context.DeadlineExceeded, err)
	}
}

}

// reading snapshot with deadline exceeded should error out
ctx, cancel = context.WithTimeout(context.Background(), 2*time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

also prefer small timeout. so test runs faster. i suggest 100ms.

@gyuho gyuho force-pushed the snapshot-error branch 2 times, most recently from 2b5a3e5 to 23f54d6 Compare December 19, 2017 18:17
@xiang90
Copy link
Contributor

xiang90 commented Dec 19, 2017

lgtm

@fanminshi
Copy link
Member

lgtm

@gyuho gyuho merged commit ecbd1ae into etcd-io:master Dec 19, 2017
@gyuho gyuho deleted the snapshot-error branch December 19, 2017 19:14
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.

3 participants