Skip to content

Conversation

@hopha95
Copy link
Contributor

@hopha95 hopha95 commented Jun 21, 2024

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

@hopha95 hopha95 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

}

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.

@hopha95
Copy link
Contributor Author

hopha95 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
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
eliben pushed a commit to eliben/google-cloud-go 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