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

feat(bigtable): support partial results in InstanceAdminClient.Cluste… #2932

Merged
merged 5 commits into from Feb 8, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions bigtable/admin.go
Expand Up @@ -1008,7 +1008,10 @@ func (iac *InstanceAdminClient) UpdateCluster(ctx context.Context, instanceID, c
return longrunning.InternalNewOperation(iac.lroClient, lro).Wait(ctx, nil)
}

// Clusters lists the clusters in an instance.
// Clusters lists the clusters in an instance. If any location
// (cluster) is unavailable due to some transient conditions, Clusters
// returns partial results and ErrPartiallyUnavailable error with
// unavailable locations list.
func (iac *InstanceAdminClient) Clusters(ctx context.Context, instanceID string) ([]*ClusterInfo, error) {
ctx = mergeOutgoingMetadata(ctx, iac.md)
req := &btapb.ListClustersRequest{Parent: "projects/" + iac.project + "/instances/" + instanceID}
Expand All @@ -1021,7 +1024,6 @@ func (iac *InstanceAdminClient) Clusters(ctx context.Context, instanceID string)
if err != nil {
return nil, err
}
// TODO(garyelliott): Deal with failed_locations.
var cis []*ClusterInfo
for _, c := range res.Clusters {
nameParts := strings.Split(c.Name, "/")
Expand All @@ -1034,6 +1036,11 @@ func (iac *InstanceAdminClient) Clusters(ctx context.Context, instanceID string)
StorageType: storageTypeFromProto(c.DefaultStorageType),
})
}
if len(res.FailedLocations) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a test?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, this would help.

Copy link
Author

Choose a reason for hiding this comment

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

Hm-m, as we're not very able to simulate "failed locations" error on real data (at least I don't know how this can be done), I think a test can be written with kind of a mock for ListClusters() method. But the mock is big, as there is an interface implementation restriction, and the interface describes a lot of methods. By the way, I see that emulator doesn't support most part of instance and cluster related methods. Should we implement them? Doesn't seem to be very hard.

// Return partial results and an error in
// case of some locations are unavailable.
return cis, ErrPartiallyUnavailable{res.FailedLocations}
}
return cis, nil
}

Expand Down
117 changes: 117 additions & 0 deletions bigtable/integration_test.go
Expand Up @@ -37,6 +37,10 @@ import (
gax "github.com/googleapis/gax-go/v2"
"google.golang.org/api/iterator"
btapb "google.golang.org/genproto/googleapis/bigtable/admin/v2"
v1 "google.golang.org/genproto/googleapis/iam/v1"
longrunning "google.golang.org/genproto/googleapis/longrunning"
grpc "google.golang.org/grpc"
"google.golang.org/protobuf/types/known/emptypb"
)

var (
Expand Down Expand Up @@ -1711,6 +1715,119 @@ func TestIntegration_AdminSnapshot(t *testing.T) {
}
}

// instanceAdminClientMock is used to test FailedLocations field processing.
type instanceAdminClientMock struct {
Clusters []*btapb.Cluster
UnavailableLocations []string
}

func (iacm *instanceAdminClientMock) ListClusters(ctx context.Context, req *btapb.ListClustersRequest, opts ...grpc.CallOption) (*btapb.ListClustersResponse, error) {
res := btapb.ListClustersResponse{
Clusters: iacm.Clusters,
FailedLocations: iacm.UnavailableLocations,
}
return &res, nil
}
Copy link
Author

Choose a reason for hiding this comment

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

This method will be called by Clusters which I'm changing with this PR.


func (iacm *instanceAdminClientMock) CreateInstance(ctx context.Context, in *btapb.CreateInstanceRequest, opts ...grpc.CallOption) (*longrunning.Operation, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) GetInstance(ctx context.Context, in *btapb.GetInstanceRequest, opts ...grpc.CallOption) (*btapb.Instance, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) ListInstances(ctx context.Context, in *btapb.ListInstancesRequest, opts ...grpc.CallOption) (*btapb.ListInstancesResponse, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) UpdateInstance(ctx context.Context, in *btapb.Instance, opts ...grpc.CallOption) (*btapb.Instance, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) PartialUpdateInstance(ctx context.Context, in *btapb.PartialUpdateInstanceRequest, opts ...grpc.CallOption) (*longrunning.Operation, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) DeleteInstance(ctx context.Context, in *btapb.DeleteInstanceRequest, opts ...grpc.CallOption) (*emptypb.Empty, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) CreateCluster(ctx context.Context, in *btapb.CreateClusterRequest, opts ...grpc.CallOption) (*longrunning.Operation, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) GetCluster(ctx context.Context, in *btapb.GetClusterRequest, opts ...grpc.CallOption) (*btapb.Cluster, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) UpdateCluster(ctx context.Context, in *btapb.Cluster, opts ...grpc.CallOption) (*longrunning.Operation, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) DeleteCluster(ctx context.Context, in *btapb.DeleteClusterRequest, opts ...grpc.CallOption) (*emptypb.Empty, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) CreateAppProfile(ctx context.Context, in *btapb.CreateAppProfileRequest, opts ...grpc.CallOption) (*btapb.AppProfile, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) GetAppProfile(ctx context.Context, in *btapb.GetAppProfileRequest, opts ...grpc.CallOption) (*btapb.AppProfile, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) ListAppProfiles(ctx context.Context, in *btapb.ListAppProfilesRequest, opts ...grpc.CallOption) (*btapb.ListAppProfilesResponse, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) UpdateAppProfile(ctx context.Context, in *btapb.UpdateAppProfileRequest, opts ...grpc.CallOption) (*longrunning.Operation, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) DeleteAppProfile(ctx context.Context, in *btapb.DeleteAppProfileRequest, opts ...grpc.CallOption) (*emptypb.Empty, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) GetIamPolicy(ctx context.Context, in *v1.GetIamPolicyRequest, opts ...grpc.CallOption) (*v1.Policy, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) SetIamPolicy(ctx context.Context, in *v1.SetIamPolicyRequest, opts ...grpc.CallOption) (*v1.Policy, error) {
return nil, nil
}
func (iacm *instanceAdminClientMock) TestIamPermissions(ctx context.Context, in *v1.TestIamPermissionsRequest, opts ...grpc.CallOption) (*v1.TestIamPermissionsResponse, error) {
return nil, nil
}

func TestIntegration_InstanceAdminClient_Clusters_WithFailedLocations(t *testing.T) {
testEnv, err := NewIntegrationEnv()
if err != nil {
t.Fatalf("IntegrationEnv: %v", err)
}
defer testEnv.Close()

if !testEnv.Config().UseProd {
t.Skip("emulator doesn't support snapshots")
}

iAdminClient, err := testEnv.NewInstanceAdminClient()
if err != nil {
t.Fatalf("NewInstanceAdminClient: %v", err)
}
defer iAdminClient.Close()

cluster1 := btapb.Cluster{Name: "cluster1"}
failedLoc := "euro1"

iAdminClient.iClient = &instanceAdminClientMock{
Clusters: []*btapb.Cluster{&cluster1},
UnavailableLocations: []string{failedLoc},
}
Copy link
Author

Choose a reason for hiding this comment

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

Setting mock data for the test.


cis, err := iAdminClient.Clusters(context.Background(), "instance-id")
convertedErr, ok := err.(ErrPartiallyUnavailable)
if !ok {
t.Fatalf("want error ErrPartiallyUnavailable, got other")
}
if got, want := len(convertedErr.Locations), 1; got != want {
t.Fatalf("want %v failed locations, got %v", want, got)
}
if got, want := convertedErr.Locations[0], failedLoc; got != want {
t.Fatalf("want failed location %v, got %v", want, got)
}
if got, want := len(cis), 1; got != want {
t.Fatalf("want %v failed locations, got %v", want, got)
}
if got, want := cis[0].Name, cluster1.Name; got != want {
t.Fatalf("want cluster %v, got %v", want, got)
}
}

func TestIntegration_Granularity(t *testing.T) {
testEnv, err := NewIntegrationEnv()
if err != nil {
Expand Down