Skip to content

make the tablet identification added to errors more concise #3015

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.

Sign up for GitHub

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

Merged
sougou merged 2 commits into from
Aug 2, 2017
Merged

make the tablet identification added to errors more concise #3015

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50d39d5
make the tablet identification added to errors more concise
demmer Jul 26, 2017
e44f126
update concise tablet logging
demmer Jul 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions go/vt/vtgate/gateway/discoverygateway_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ import (
vtrpcpb "github.com/youtube/vitess/go/vt/proto/vtrpc"
)

func tabletIdent(tablet *topodatapb.Tablet) string {
return fmt.Sprintf("%s-%d (%s)", tablet.Alias.Cell, tablet.Alias.Uid, tablet.Hostname)
}

@alainjobart alainjobart Jul 26, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's move that function to go/vt/topotools/tablet.go, export it and name it TabletIdent.

Additionally, it would be nice to also have in there the 'tags' fields, something like:
cell-123 (host1 tag1=value1)
The tags are usually used to describe a tablet (and for instance put some extra name in there, that's what we do inside YouTube). They are displayed in 'vtctl ListAllTablets' and it's very useful.

func TestDiscoveryGatewayExecute(t *testing.T) {
testDiscoveryGatewayGeneric(t, false, func(dg Gateway, target *querypb.Target) error {
_, err := dg.Execute(context.Background(), target, "query", nil, 0, nil)
Expand Down Expand Up @@ -171,8 +175,8 @@ func testDiscoveryGatewayGeneric(t *testing.T, streaming bool, f func(dg Gateway
ep1 = sc1.Tablet()
ep2 := sc2.Tablet()
wants := map[string]int{
fmt.Sprintf(`target: ks.0.replica, used tablet: (%+v), FAILED_PRECONDITION error`, ep1): 0,
fmt.Sprintf(`target: ks.0.replica, used tablet: (%+v), FAILED_PRECONDITION error`, ep2): 0,
fmt.Sprintf(`target: ks.0.replica, used tablet: %s, FAILED_PRECONDITION error`, tabletIdent(ep1)): 0,
fmt.Sprintf(`target: ks.0.replica, used tablet: %s, FAILED_PRECONDITION error`, tabletIdent(ep2)): 0,
}
err = f(dg, target)
if _, ok := wants[fmt.Sprintf("%v", err)]; !ok {
Expand All @@ -189,8 +193,8 @@ func testDiscoveryGatewayGeneric(t *testing.T, streaming bool, f func(dg Gateway
ep1 = sc1.Tablet()
ep2 = sc2.Tablet()
wants = map[string]int{
fmt.Sprintf(`target: ks.0.replica, used tablet: (%+v), FAILED_PRECONDITION error`, ep1): 0,
fmt.Sprintf(`target: ks.0.replica, used tablet: (%+v), FAILED_PRECONDITION error`, ep2): 0,
fmt.Sprintf(`target: ks.0.replica, used tablet: %s, FAILED_PRECONDITION error`, tabletIdent(ep1)): 0,
fmt.Sprintf(`target: ks.0.replica, used tablet: %s, FAILED_PRECONDITION error`, tabletIdent(ep2)): 0,
}
err = f(dg, target)
if _, ok := wants[fmt.Sprintf("%v", err)]; !ok {
Expand All @@ -203,7 +207,7 @@ func testDiscoveryGatewayGeneric(t *testing.T, streaming bool, f func(dg Gateway
sc1 = hc.AddTestTablet("cell", "1.1.1.1", 1001, keyspace, shard, tabletType, true, 10, nil)
sc1.MustFailCodes[vtrpcpb.Code_INVALID_ARGUMENT] = 1
ep1 = sc1.Tablet()
want = fmt.Sprintf(`target: ks.0.replica, used tablet: (%+v), INVALID_ARGUMENT error`, ep1)
want = fmt.Sprintf(`target: ks.0.replica, used tablet: %s, INVALID_ARGUMENT error`, tabletIdent(ep1))
err = f(dg, target)
verifyShardError(t, err, want, vtrpcpb.Code_INVALID_ARGUMENT)

Expand Down Expand Up @@ -239,8 +243,8 @@ func testDiscoveryGatewayTransact(t *testing.T, streaming bool, f func(dg Gatewa
ep1 := sc1.Tablet()
ep2 := sc2.Tablet()
wants := map[string]int{
fmt.Sprintf(`target: ks.0.replica, used tablet: (%+v), FAILED_PRECONDITION error`, ep1): 0,
fmt.Sprintf(`target: ks.0.replica, used tablet: (%+v), FAILED_PRECONDITION error`, ep2): 0,
fmt.Sprintf(`target: ks.0.replica, used tablet: %s, FAILED_PRECONDITION error`, tabletIdent(ep1)): 0,
fmt.Sprintf(`target: ks.0.replica, used tablet: %s, FAILED_PRECONDITION error`, tabletIdent(ep2)): 0,
}
err := f(dg, target)
if _, ok := wants[fmt.Sprintf("%v", err)]; !ok {
Expand All @@ -253,7 +257,7 @@ func testDiscoveryGatewayTransact(t *testing.T, streaming bool, f func(dg Gatewa
sc1 = hc.AddTestTablet("cell", "1.1.1.1", 1001, keyspace, shard, tabletType, true, 10, nil)
sc1.MustFailCodes[vtrpcpb.Code_INVALID_ARGUMENT] = 1
ep1 = sc1.Tablet()
want := fmt.Sprintf(`target: ks.0.replica, used tablet: (%+v), INVALID_ARGUMENT error`, ep1)
want := fmt.Sprintf(`target: ks.0.replica, used tablet: %s, INVALID_ARGUMENT error`, tabletIdent(ep1))
err = f(dg, target)
verifyShardError(t, err, want, vtrpcpb.Code_INVALID_ARGUMENT)
}
Expand Down
2 changes: 1 addition & 1 deletion go/vt/vtgate/gateway/shard_error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func NewShardError(in error, target *querypb.Target, tablet *topodatapb.Tablet,
return nil
}
if tablet != nil {
return vterrors.Errorf(vterrors.Code(in), "target: %s.%s.%s, used tablet: (%+v), %v", target.Keyspace, target.Shard, topoproto.TabletTypeLString(target.TabletType), tablet, in)
return vterrors.Errorf(vterrors.Code(in), "target: %s.%s.%s, used tablet: %s-%d (%s), %v", target.Keyspace, target.Shard, topoproto.TabletTypeLString(target.TabletType), tablet.Alias.Cell, tablet.Alias.Uid, tablet.Hostname, in)

@alainjobart alainjobart Jul 26, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And then you can use TabletIndent here too.

}
return vterrors.Errorf(vterrors.Code(in), "target: %s.%s.%s, %v", target.Keyspace, target.Shard, topoproto.TabletTypeLString(target.TabletType), in)
}
14 changes: 7 additions & 7 deletions go/vt/vtgate/resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,8 @@ func testResolverGeneric(t *testing.T, name string, action func(res *Resolver) (
sbc0.MustFailCodes[vtrpcpb.Code_INVALID_ARGUMENT] = 1
sbc1.MustFailCodes[vtrpcpb.Code_INTERNAL] = 1
_, err = action(res)
want1 := fmt.Sprintf("target: %s.-20.master, used tablet: (alias:<cell:\"aa\" > hostname:\"-20\" port_map:<key:\"vt\" value:1 > keyspace:\"%s\" shard:\"-20\" type:MASTER ), INVALID_ARGUMENT error", name, name)
want2 := fmt.Sprintf("target: %s.20-40.master, used tablet: (alias:<cell:\"aa\" > hostname:\"20-40\" port_map:<key:\"vt\" value:1 > keyspace:\"%s\" shard:\"20-40\" type:MASTER ), INTERNAL error", name, name)
want1 := fmt.Sprintf("target: %s.-20.master, used tablet: aa-0 (-20), INVALID_ARGUMENT error", name)
want2 := fmt.Sprintf("target: %s.20-40.master, used tablet: aa-0 (20-40), INTERNAL error", name)
want := []string{want1, want2}
sort.Strings(want)
if err == nil {
Expand Down Expand Up @@ -247,8 +247,8 @@ func testResolverGeneric(t *testing.T, name string, action func(res *Resolver) (
sbc0.MustFailCodes[vtrpcpb.Code_FAILED_PRECONDITION] = 1
sbc1.MustFailCodes[vtrpcpb.Code_FAILED_PRECONDITION] = 1
_, err = action(res)
want1 = fmt.Sprintf("target: %s.-20.master, used tablet: (alias:<cell:\"aa\" > hostname:\"-20\" port_map:<key:\"vt\" value:1 > keyspace:\"%s\" shard:\"-20\" type:MASTER ), FAILED_PRECONDITION error", name, name)
want2 = fmt.Sprintf("target: %s.20-40.master, used tablet: (alias:<cell:\"aa\" > hostname:\"20-40\" port_map:<key:\"vt\" value:1 > keyspace:\"%s\" shard:\"20-40\" type:MASTER ), FAILED_PRECONDITION error", name, name)
want1 = fmt.Sprintf("target: %s.-20.master, used tablet: aa-0 (-20), FAILED_PRECONDITION error", name)
want2 = fmt.Sprintf("target: %s.20-40.master, used tablet: aa-0 (20-40), FAILED_PRECONDITION error", name)
want = []string{want1, want2}
sort.Strings(want)
if err == nil {
Expand Down Expand Up @@ -392,7 +392,7 @@ func testResolverStreamGeneric(t *testing.T, name string, action func(res *Resol
hc.AddTestTablet("aa", "20-40", 1, name, "20-40", topodatapb.TabletType_MASTER, true, 1, nil)
sbc0.MustFailCodes[vtrpcpb.Code_INTERNAL] = 1
_, err = action(res)
want := fmt.Sprintf("target: %s.-20.master, used tablet: (alias:<cell:\"aa\" > hostname:\"-20\" port_map:<key:\"vt\" value:1 > keyspace:\"%s\" shard:\"-20\" type:MASTER ), INTERNAL error", name, name)
want := fmt.Sprintf("target: %s.-20.master, used tablet: aa-0 (-20), INTERNAL error", name)
if err == nil || err.Error() != want {
t.Errorf("want\n%s\ngot\n%v", want, err)
}
Expand Down Expand Up @@ -611,7 +611,7 @@ func TestResolverExecBatchReresolve(t *testing.T) {
}

_, err := res.ExecuteBatch(context.Background(), topodatapb.TabletType_MASTER, false, nil, nil, buildBatchRequest)
want := "target: TestResolverExecBatchReresolve.0.master, used tablet: (alias:<cell:\"aa\" > hostname:\"0\" port_map:<key:\"vt\" value:1 > keyspace:\"TestResolverExecBatchReresolve\" shard:\"0\" type:MASTER ), FAILED_PRECONDITION error"
want := "target: TestResolverExecBatchReresolve.0.master, used tablet: aa-0 (0), FAILED_PRECONDITION error"
if err == nil || err.Error() != want {
t.Errorf("want %s, got %v", want, err)
}
Expand Down Expand Up @@ -648,7 +648,7 @@ func TestResolverExecBatchAsTransaction(t *testing.T) {
}

_, err := res.ExecuteBatch(context.Background(), topodatapb.TabletType_MASTER, true, nil, nil, buildBatchRequest)
want := "target: TestResolverExecBatchAsTransaction.0.master, used tablet: (alias:<cell:\"aa\" > hostname:\"0\" port_map:<key:\"vt\" value:1 > keyspace:\"TestResolverExecBatchAsTransaction\" shard:\"0\" type:MASTER ), INTERNAL error"
want := "target: TestResolverExecBatchAsTransaction.0.master, used tablet: aa-0 (0), INTERNAL error"
if err == nil || err.Error() != want {
t.Errorf("want %v, got %v", want, err)
}
Expand Down
6 changes: 3 additions & 3 deletions go/vt/vtgate/scatter_conn_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func testScatterConnGeneric(t *testing.T, name string, f func(sc *ScatterConn, s
sbc := hc.AddTestTablet("aa", "0", 1, name, "0", topodatapb.TabletType_REPLICA, true, 1, nil)
sbc.MustFailCodes[vtrpcpb.Code_INVALID_ARGUMENT] = 1
qr, err = f(sc, []string{"0"})
want := fmt.Sprintf("target: %v.0.replica, used tablet: (alias:<cell:\"aa\" > hostname:\"0\" port_map:<key:\"vt\" value:1 > keyspace:\"%s\" shard:\"0\" type:REPLICA ), INVALID_ARGUMENT error", name, name)
want := fmt.Sprintf("target: %v.0.replica, used tablet: aa-0 (0), INVALID_ARGUMENT error", name)
// Verify server error string.
if err == nil || err.Error() != want {
t.Errorf("want %s, got %v", want, err)
Expand All @@ -159,7 +159,7 @@ func testScatterConnGeneric(t *testing.T, name string, f func(sc *ScatterConn, s
sbc1.MustFailCodes[vtrpcpb.Code_INVALID_ARGUMENT] = 1
_, err = f(sc, []string{"0", "1"})
// Verify server errors are consolidated.
want = fmt.Sprintf("target: %v.0.replica, used tablet: (alias:<cell:\"aa\" > hostname:\"0\" port_map:<key:\"vt\" value:1 > keyspace:\"%v\" shard:\"0\" type:REPLICA ), INVALID_ARGUMENT error\ntarget: %v.1.replica, used tablet: (alias:<cell:\"aa\" > hostname:\"1\" port_map:<key:\"vt\" value:1 > keyspace:\"%v\" shard:\"1\" type:REPLICA ), INVALID_ARGUMENT error", name, name, name, name)
want = fmt.Sprintf("target: %v.0.replica, used tablet: aa-0 (0), INVALID_ARGUMENT error\ntarget: %v.1.replica, used tablet: aa-0 (1), INVALID_ARGUMENT error", name, name)
verifyScatterConnError(t, err, want, vtrpcpb.Code_INVALID_ARGUMENT)
// Ensure that we tried only once.
if execCount := sbc0.ExecCount.Get(); execCount != 1 {
Expand All @@ -179,7 +179,7 @@ func testScatterConnGeneric(t *testing.T, name string, f func(sc *ScatterConn, s
sbc1.MustFailCodes[vtrpcpb.Code_RESOURCE_EXHAUSTED] = 1
_, err = f(sc, []string{"0", "1"})
// Verify server errors are consolidated.
want = fmt.Sprintf("target: %v.0.replica, used tablet: (alias:<cell:\"aa\" > hostname:\"0\" port_map:<key:\"vt\" value:1 > keyspace:\"%v\" shard:\"0\" type:REPLICA ), INVALID_ARGUMENT error\ntarget: %v.1.replica, used tablet: (alias:<cell:\"aa\" > hostname:\"1\" port_map:<key:\"vt\" value:1 > keyspace:\"%v\" shard:\"1\" type:REPLICA ), RESOURCE_EXHAUSTED error", name, name, name, name)
want = fmt.Sprintf("target: %v.0.replica, used tablet: aa-0 (0), INVALID_ARGUMENT error\ntarget: %v.1.replica, used tablet: aa-0 (1), RESOURCE_EXHAUSTED error", name, name)
// We should only surface the higher priority error code
verifyScatterConnError(t, err, want, vtrpcpb.Code_INVALID_ARGUMENT)
// Ensure that we tried only once.
Expand Down