Add support for Google Cloud Bigtable. #177
Conversation
…ng object properties.
|
Assuming nothing's changed since the other reviews, 👍 |
| "num_nodes": { | ||
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| ForceNew: true, |
There was a problem hiding this comment.
This value can't be updated? Seems like a natural thing to want to adjust later
There was a problem hiding this comment.
We can't update it with the client as-is, or even read it for that matter. When the client supports updating this value, we can remove ForceNew.
google/bigtable_client_factory.go
Outdated
| TokenSource oauth2.TokenSource | ||
| } | ||
|
|
||
| func (s *BigtableClientFactory) NewInstanceAdminClient(project string) (*bigtable.InstanceAdminClient, error) { |
There was a problem hiding this comment.
If you remove the pointer on the receiver in the signature (e.g.
func (s BigtableClientFactory) NewInstanceAdminClient(project string) (*bigtable.InstanceAdminClient, error) {
), then this function is much more explicit that it is immutable (and is therefore thread safe, etc)
google/bigtable_client_factory.go
Outdated
| return bigtable.NewInstanceAdminClient(context.Background(), project, option.WithTokenSource(s.TokenSource), option.WithUserAgent(s.UserAgent)) | ||
| } | ||
|
|
||
| func (s *BigtableClientFactory) NewAdminClient(project, instance string) (*bigtable.AdminClient, error) { |
| return fmt.Errorf("Error starting instance admin client. %s", err) | ||
| } | ||
|
|
||
| defer c.Close() |
There was a problem hiding this comment.
I think golang tooling gets mad if you don't check the return result here in case it returns an error
There was a problem hiding this comment.
I haven't had Gogland get mad at me, and I don't think we can do anything useful with an error from this - any interesting errors would have come from operations we performed.
There was a problem hiding this comment.
Yeah agree, the only thing we can do is log it but if we're closing it I doubt we're interested in the log message 👍
…pointers to struct.
…pport Add support for Google Cloud Bigtable.
…pport Add support for Google Cloud Bigtable.
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
Merge the
bigtable-supportbranch intomaster.Add support for two new resources:
google_bigtable_instance- A container for a single Bigtable cluster.google_bigtable_table- A Bigtable table contained within an instance.