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 update column family's value type to non-aggregate type #10410

Merged
merged 13 commits into from
Jul 1, 2024

Conversation

hoangpham95
Copy link
Contributor

@hoangpham95 hoangpham95 commented Jun 21, 2024

BEGIN_COMMIT_OVERRIDE
chore(bigtable): reverted Support update column family's value type to non-aggregate type
END_COMMIT_OVERRIDE

@hoangpham95 hoangpham95 requested review from a team as code owners June 21, 2024 16:41
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Jun 21, 2024
@bhshkh
Copy link
Contributor

bhshkh commented Jun 25, 2024

Add integration test

@@ -707,13 +707,38 @@ func (ac *AdminClient) setGCPolicy(ctx context.Context, table, family string, po
return err
}

func (ac *AdminClient) setValueTypeImpl(ctx context.Context, table, family string, valueType Type) error {
if _, ok := valueType.proto().GetKind().(*btapb.Type_AggregateType); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Check valueType nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it needed? valueType is Type and not pointer to Type so it should always be non-null, right?

Copy link
Contributor

@bhshkh bhshkh Jun 27, 2024

Choose a reason for hiding this comment

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

In the test, i passed nil

err := c.SetValueType(context.Background(), "My-table", "cf1", nil)
		if err != nil && !test.hasError {
			t.Fatalf("Unexpected error when setting type: %v", err)
		}

and I don't get any compile time error but see a panic when test is run:

--- FAIL: TestTableAdmin_SetType (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x10d7942]

goroutine 10 [running]:
testing.tRunner.func1.2({0x129d780, 0x2139e20})
	/usr/lib/google-golang/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
	/usr/lib/google-golang/src/testing/testing.go:1634 +0x377
panic({0x129d780?, 0x2139e20?})
	/usr/lib/google-golang/src/runtime/panic.go:759 +0x132
cloud.google.com/go/bigtable.(*AdminClient).setValueTypeImpl(0xc000138c80, {0x16876d0, 0x21a42c0}, {0x14e794e, 0x8}, {0x14e3557, 0x3}, {0x0, 0x0})
	/usr/local/google/home/bahaaiman/Documents/cfdb-workspace-01/hoang/google-cloud-go/bigtable/admin.go:711 +0x62
cloud.google.com/go/bigtable.(*AdminClient).SetValueType(...)
	/usr/local/google/home/bahaaiman/Documents/cfdb-workspace-01/hoang/google-cloud-go/bigtable/admin.go:739
cloud.google.com/go/bigtable.TestTableAdmin_SetType(0xc000124820)
	/usr/local/google/home/bahaaiman/Documents/cfdb-workspace-01/hoang/google-cloud-go/bigtable/admin_test.go:433 +0x205
testing.tRunner(0xc000124820, 0x1556ea0)
	/usr/lib/google-golang/src/testing/testing.go:1689 +0xfb
created by testing.(*T).Run in goroutine 1
	/usr/lib/google-golang/src/testing/testing.go:1742 +0x390
FAIL	cloud.google.com/go/bigtable	0.156s
FAIL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a nil check.

@hoangpham95
Copy link
Contributor Author

hoangpham95 commented Jun 27, 2024

Add integration test

Done:

go test -test.run="TestIntegration_TestUpdateColumnFamilyValueType" -v \
> -it.use-prod \
> -it.admin-endpoint="test-bigtableadmin.sandbox.googleapis.com:443" \
> -it.data-endpoint="test-bigtable.sandbox.googleapis.com:443" \
> -it.project="google.com:cloud-bigtable-dev" \
> -it.cluster="phhoang-btql-test-c1" \
> -it.instance="phhoang-btql-test"
Note: when using prod, you must first create an instance:
cbt createinstance phhoang-btql-test phhoang-btql-test phhoang-btql-test-c1 us-central1-b 1 SSD
=== RUN   TestIntegration_TestUpdateColumnFamilyValueType
--- PASS: TestIntegration_TestUpdateColumnFamilyValueType (6.92s)
PASS

ok      cloud.google.com/go/bigtable    59.654s

@bhshkh bhshkh enabled auto-merge (squash) July 1, 2024 17:56
@bhshkh bhshkh merged commit df60464 into googleapis:main Jul 1, 2024
8 of 10 checks passed
bhshkh added a commit to bhshkh/google-cloud-go that referenced this pull request Jul 22, 2024
bhshkh added a commit that referenced this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants