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
Changes from 34 commits
ac36180
42bd697
ab2a888
66cc276
3449326
0e148b6
88764e6
c2e05a0
326170f
9304668
4cc0f8c
9445f55
23af05d
573f0fa
982b8e1
5416687
2245185
a5027e8
f84aa6c
b557f4e
1b259a2
f3ad7cd
316ab3a
0bcff05
a608feb
f053e97
799a840
532b9ab
024cf86
7fcf8b6
b7f9e08
650b653
a380536
9beb501
91f44d3
c48e38d
80255fd
fd1ee0d
f444a27
97a7266
a25bff1
d2b768a
4d5ceee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Document this? Wasn't immediately clear to me what f was for. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 atableLike
object, which simply hasReadRows
. (I think I expand it in another PR, IIRC :) )Normally,
getTable
callsgetClient(clientConf).Open(tableName)
, but if the globaltable
is set, it returns that.timeout_test.go
setstable
to a mock that captures the context passed toReadRows
so we can assert that it was set correctly.