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/cmd/cbt): Add a timeout option #4276

Merged
merged 43 commits into from Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
ac36180
Implemented a -timeout option.
jimfulton Jun 15, 2021
42bd697
Merge remote-tracking branch 'upstream/master' into timeout
jimfulton Jun 17, 2021
ab2a888
gofmt
jimfulton Jun 17, 2021
66cc276
Make timeout a config setting, not just a flag and also add to docs
jimfulton Jun 17, 2021
3449326
Merge branch 'master' into timeout
jimfulton Jun 17, 2021
0e148b6
Merge branch 'master' into timeout
jimfulton Jun 17, 2021
88764e6
Refactor a little to make functional testing practical.
jimfulton Jun 17, 2021
c2e05a0
testing helpers
jimfulton Jun 17, 2021
326170f
Test that timeouts are plumbed properly
jimfulton Jun 17, 2021
9304668
refactored stdio capture to be in-memory
jimfulton Jun 17, 2021
4cc0f8c
gofmt/lint
jimfulton Jun 17, 2021
9445f55
Merge remote-tracking branch 'origin/timeout' into timeout
jimfulton Jun 17, 2021
23af05d
Merge branch 'master' into timeout
jimfulton Jun 17, 2021
573f0fa
Added missing licence headers
jimfulton Jun 18, 2021
982b8e1
Merge remote-tracking branch 'origin/timeout' into timeout
jimfulton Jun 18, 2021
5416687
Merge branch 'master' into timeout
jimfulton Jun 18, 2021
2245185
document captureStdout
jimfulton Jun 28, 2021
a5027e8
Merge branch 'timeout' of github.com:jimfulton/google-cloud-go into t…
jimfulton Jun 28, 2021
f84aa6c
Merge branch 'master' into timeout
jimfulton Jun 28, 2021
b557f4e
Merge branch 'master' into timeout
jimfulton Jun 29, 2021
1b259a2
Merge branch 'master' into timeout
jimfulton Jul 1, 2021
f3ad7cd
Merge branch 'master' into timeout
jimfulton Jul 7, 2021
316ab3a
Merge branch 'master' into timeout
jimfulton Jul 8, 2021
0bcff05
Merge branch 'master' into timeout
jimfulton Jul 8, 2021
a608feb
Merge branch 'master' into timeout
jimfulton Jul 9, 2021
f053e97
Merge branch 'master' into timeout
jimfulton Jul 9, 2021
799a840
Merge branch 'master' into timeout
jimfulton Jul 12, 2021
532b9ab
Merge branch 'master' into timeout
jimfulton Jul 12, 2021
024cf86
Merge branch 'master' into timeout
jimfulton Jul 13, 2021
7fcf8b6
Merge branch 'master' into timeout
jimfulton Jul 15, 2021
b7f9e08
Merge branch 'master' into timeout
jimfulton Jul 22, 2021
650b653
Merge branch 'master' into timeout
crwilcox Jul 27, 2021
a380536
Merge branch 'master' into timeout
jimfulton Jul 27, 2021
9beb501
Merge branch 'master' into timeout
crwilcox Jul 27, 2021
91f44d3
fix errors.
jimfulton Jul 27, 2021
c48e38d
Merge branch 'master' into timeout
jimfulton Jul 27, 2021
80255fd
Merge branch 'master' into timeout
jimfulton Jul 29, 2021
fd1ee0d
Merge branch 'master' into timeout
jimfulton Jul 30, 2021
f444a27
Merge branch 'master' into timeout
jimfulton Aug 2, 2021
97a7266
Merge branch 'master' into timeout
jimfulton Aug 5, 2021
a25bff1
Merge branch 'master' into timeout
jimfulton Aug 8, 2021
d2b768a
Merge branch 'master' into timeout
jimfulton Aug 10, 2021
4d5ceee
Merge branch 'master' into timeout
crwilcox Aug 10, 2021
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
38 changes: 32 additions & 6 deletions bigtable/cmd/cbt/cbt.go
Expand Up @@ -49,6 +49,7 @@ var (

config *cbtconfig.Config
client *bigtable.Client
table tableLike
adminClient *bigtable.AdminClient
instanceAdminClient *bigtable.InstanceAdminClient

Expand All @@ -58,6 +59,10 @@ var (
cliUserAgent = "cbt-cli-go/unknown"
)

type tableLike interface {
ReadRows(ctx context.Context, arg bigtable.RowSet, f func(bigtable.Row) bool, opts ...bigtable.ReadOption) (err error)
}

func getCredentialOpts(opts []option.ClientOption) []option.ClientOption {
if ts := config.TokenSource; ts != nil {
opts = append(opts, option.WithTokenSource(ts))
Expand Down Expand Up @@ -85,6 +90,14 @@ func getClient(clientConf bigtable.ClientConfig) *bigtable.Client {
return client
}

func getTable(clientConf bigtable.ClientConfig, tableName string) tableLike {
if table != nil {
return table
}
table = getClient(clientConf).Open(tableName)
return table
}

func getAdminClient() *bigtable.AdminClient {
if adminClient == nil {
var opts []option.ClientOption
Expand Down Expand Up @@ -146,25 +159,37 @@ func main() {
os.Stdout = f
}

doMain(config, flag.Args())
}

func doMain(config *cbtconfig.Config, args []string) {
if config.UserAgent != "" {
cliUserAgent = config.UserAgent
}

ctx := context.Background()
var ctx context.Context
if config.Timeout > 0 {
var cancel context.CancelFunc
ctx, cancel = context.WithTimeout(context.Background(), config.Timeout)
defer cancel()
} else {
ctx = context.Background()
}

if config.AuthToken != "" {
ctx = metadata.AppendToOutgoingContext(ctx, "x-goog-iam-authorization-token", config.AuthToken)
}

for _, cmd := range commands {
if cmd.Name == flag.Arg(0) {
if cmd.Name == args[0] {
if err := config.CheckFlags(cmd.Required); err != nil {
log.Fatal(err)
}
cmd.do(ctx, flag.Args()[1:]...)
cmd.do(ctx, args[1:]...)
return
}
}
log.Fatalf("Unknown command %q", flag.Arg(0))
log.Fatalf("Unknown command %q", args[0])
}

func usage(w io.Writer) {
Expand Down Expand Up @@ -212,6 +237,7 @@ options to your ~/.cbtrc file in the following format:
admin-endpoint = hostname:port
data-endpoint = hostname:port
auth-token = AJAvW039NO1nDcijk_J6_rFXG_...
timeout = 30s

All values are optional and can be overridden at the command prompt.
`
Expand Down Expand Up @@ -581,7 +607,7 @@ func doCount(ctx context.Context, args ...string) {
if len(args) != 1 {
log.Fatal("usage: cbt count <table>")
}
tbl := getClient(bigtable.ClientConfig{}).Open(args[0])
tbl := getTable(bigtable.ClientConfig{}, args[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

I am having some difficulty fully understanding the need of this refactoring and its relation to the timeout change. Can you describe it a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to test that a proper context (with a timeout) was passed to ReadRows.

getTable returns a tableLike object, which simply has ReadRows. (I think I expand it in another PR, IIRC :) )

Normally, getTable calls getClient(clientConf).Open(tableName), but if the global table is set, it returns that.

timeout_test.go sets table to a mock that captures the context passed to ReadRows so we can assert that it was set correctly.


n := 0
err := tbl.ReadRows(ctx, bigtable.InfiniteRange(""), func(_ bigtable.Row) bool {
Expand Down Expand Up @@ -841,7 +867,7 @@ func doMDDoc(ctx context.Context, args ...string) { doMDDocFn(ctx, args...) }
func docFlags() []*flag.Flag {
// Only include specific flags, in a specific order.
var flags []*flag.Flag
for _, name := range []string{"project", "instance", "creds"} {
for _, name := range []string{"project", "instance", "creds", "timeout"} {
f := flag.Lookup(name)
if f == nil {
log.Fatalf("Flag not linked: -%s", name)
Expand Down
3 changes: 3 additions & 0 deletions bigtable/cmd/cbt/cbtdoc.go
Expand Up @@ -71,6 +71,8 @@ The options are:
Cloud Bigtable instance
-creds string
Path to the credentials file. If set, uses the application credentials in this file
-timeout string
Timeout (e.g. 10s, 100ms, 5m )

Example: cbt -instance=my-instance ls

Expand Down Expand Up @@ -100,6 +102,7 @@ options to your ~/.cbtrc file in the following format:
admin-endpoint = hostname:port
data-endpoint = hostname:port
auth-token = AJAvW039NO1nDcijk_J6_rFXG_...
timeout = 30s

All values are optional and can be overridden at the command prompt.

Expand Down
50 changes: 50 additions & 0 deletions bigtable/cmd/cbt/testing.go
@@ -0,0 +1,50 @@
/*
Copyright 2021 Google LLC

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package main

import (
"bytes"
"io"
"os"
)

func captureStdout(f func()) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Document this? Wasn't immediately clear to me what f was for.

Copy link
Contributor Author

@jimfulton jimfulton Jun 28, 2021

Choose a reason for hiding this comment

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

Done.

Note that here, we're mainly preventing spurious print output. In other branches/PRs, we're making assertions about captured stdout.

/*
Capture standard output to facilitate testing code that prints

or useless print output in running tests.
*/
saved := os.Stdout
r, w, _ := os.Pipe()
os.Stdout = w
defer func() { os.Stdout = saved }()

outC := make(chan string)
// https://stackoverflow.com/questions/10473800/in-go-how-do-i-capture-stdout-of-a-function-into-a-string
// copy the output in a separate goroutine so printing can't block indefinitely
go func() {
var buf bytes.Buffer
io.Copy(&buf, r)
outC <- buf.String()
}()

f()

// back to normal state
w.Close()
return <-outC
}
67 changes: 67 additions & 0 deletions bigtable/cmd/cbt/timeout_test.go
@@ -0,0 +1,67 @@
/*
Copyright 2021 Google LLC

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package main

import (
"context"
"testing"
"time"

"cloud.google.com/go/bigtable"
"cloud.google.com/go/bigtable/internal/cbtconfig"
)

type ctxTable struct {
ctx context.Context
}

func (ct *ctxTable) ReadRows(
ctx context.Context,
arg bigtable.RowSet,
f func(bigtable.Row) bool,
opts ...bigtable.ReadOption,
) (err error) {
ct.ctx = ctx
return nil
}

func TestTimeout(t *testing.T) {
ctxt := ctxTable{}
table = &ctxt
defer func() { table = nil }()

config := cbtconfig.Config{Creds: "c", Project: "p", Instance: "i"}
captureStdout(func() { doMain(&config, []string{"count", "mytable"}) })

_, deadlineSet := ctxt.ctx.Deadline()
if deadlineSet {
t.Errorf("Deadline set with no timeout")
}

config.Timeout = time.Duration(42e9)
now := time.Now()
captureStdout(func() { doMain(&config, []string{"count", "mytable"}) })

deadline, deadlineSet := ctxt.ctx.Deadline()
if !deadlineSet {
t.Errorf("Deadline set with no timeout")
}
timeout := deadline.Sub(now).Nanoseconds()
if !(timeout > 42e9 && timeout < 43e9) {
t.Errorf("Bad actual timeout nanoseconds %d", timeout)
}
}
9 changes: 9 additions & 0 deletions bigtable/internal/cbtconfig/cbtconfig.go
Expand Up @@ -47,6 +47,7 @@ type Config struct {
CertFile string // optional
UserAgent string // optional
AuthToken string // optional
Timeout time.Duration // optional
TokenSource oauth2.TokenSource // derived
TLSCreds credentials.TransportCredentials // derived
}
Expand Down Expand Up @@ -76,6 +77,8 @@ func (c *Config) RegisterFlags() {
flag.StringVar(&c.CertFile, "cert-file", c.CertFile, "Override the TLS certificates file")
flag.StringVar(&c.UserAgent, "user-agent", c.UserAgent, "Override the user agent string")
flag.StringVar(&c.AuthToken, "auth-token", c.AuthToken, "if set, use IAM Auth Token for requests")
flag.DurationVar(&c.Timeout, "timeout", c.Timeout,
"Timeout (e.g. 10s, 100ms, 5m )")
}

// CheckFlags checks that the required config values are set.
Expand Down Expand Up @@ -163,6 +166,12 @@ func readConfig(s *bufio.Scanner, filename string) (*Config, error) {
c.UserAgent = val
case "auth-token":
c.AuthToken = val
case "timeout":
timeout, err := time.ParseDuration(val)
if err != nil {
return nil, err
}
c.Timeout = timeout
}

}
Expand Down
6 changes: 6 additions & 0 deletions bigtable/internal/cbtconfig/cbtconfig_test.go
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"strings"
"testing"
"time"
)

func TestReadConfig(t *testing.T) {
Expand All @@ -32,11 +33,13 @@ func TestReadConfig(t *testing.T) {
certificateFile := "test-certificate-file"
userAgent := "test-user-agent"
authToken := "test-auth-token="
timeout := time.Duration(42e9)
// Read configuration from string containing spaces, tabs and empty lines.
validConfig := fmt.Sprintf(`
project=%s
instance=%s
creds=%s
timeout=42s

admin-endpoint =%s
data-endpoint= %s
Expand Down Expand Up @@ -72,6 +75,9 @@ func TestReadConfig(t *testing.T) {
if g, w := c.AuthToken, authToken; g != w {
t.Errorf("AuthToken mismatch\nGot: %s\nWant: %s", g, w)
}
if g, w := c.Timeout, timeout; g != w {
t.Errorf("AuthToken mismatch\nGot: %s\nWant: %s", g, w)
}

// Try to read an invalid config file and verify that it fails.
unknownKey := fmt.Sprintf("%s\nunknown-key=some-value", validConfig)
Expand Down