-
Notifications
You must be signed in to change notification settings - Fork 14
fix: add timeout and disable option for BigQuery Storage Read API #171
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
base: main
Are you sure you want to change the base?
Changes from all commits
82f90f7
812a63c
91ce1aa
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 |
|---|---|---|
|
|
@@ -26,9 +26,11 @@ import ( | |
| "bytes" | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "log" | ||
| "log/slog" | ||
| "sync/atomic" | ||
| "time" | ||
|
|
||
| "cloud.google.com/go/bigquery" | ||
| "github.com/apache/arrow-adbc/go/adbc" | ||
|
|
@@ -61,7 +63,19 @@ func checkContext(ctx context.Context, maybeErr error) error { | |
| return ctx.Err() | ||
| } | ||
|
|
||
| func runQuery(ctx context.Context, logger *slog.Logger, query *bigquery.Query, executeUpdate bool) (bigquery.ArrowIterator, int64, error) { | ||
| func runQuery(ctx context.Context, logger *slog.Logger, queryBackendAPI string, query *bigquery.Query, executeUpdate bool) (bigquery.ArrowIterator, int64, error) { | ||
| // Jobs API backend is not yet implemented (see issue #66). Fail fast with | ||
| // a clear error rather than hanging or returning a misleading permission | ||
| // error downstream. | ||
| if !executeUpdate && queryBackendAPI == OptionValueQueryBackendAPIJobs { | ||
| return nil, -1, adbc.Error{ | ||
| Code: adbc.StatusNotImplemented, | ||
| Msg: "[bq] " + OptionStringQueryBackendAPI + "=" + OptionValueQueryBackendAPIJobs + | ||
| " is not yet implemented. The REST-based fallback for reading query results has not been added to this driver (see issue #66). " + | ||
| "Use " + OptionStringQueryBackendAPI + "=" + OptionValueQueryBackendAPIStorageRead + " (the default) until the fallback is available.", | ||
| } | ||
| } | ||
|
|
||
| job, err := query.Run(ctx) | ||
| if err != nil { | ||
| return nil, -1, errToAdbcErr(adbc.StatusInternal, err, "run query") | ||
|
|
@@ -122,8 +136,47 @@ func runQuery(ctx context.Context, logger *slog.Logger, query *bigquery.Query, e | |
| Msg: "[bq] Arrow reader requires roles/bigquery.readSessionUser, see https://github.com/apache/arrow-adbc/issues/3282", | ||
| } | ||
| } | ||
| if arrowIterator, err = iter.ArrowIterator(); err != nil { | ||
| return nil, -1, errToAdbcErr(adbc.StatusInternal, err, "read Arrow query results") | ||
|
|
||
| // ArrowIterator() opens a gRPC read session via BigQuery Storage API | ||
| // (CreateReadSession RPC). The call does not accept a context, and | ||
| // has been observed to hang indefinitely in some environments. Apply | ||
| // a 30s timeout so the caller gets a clear error instead of blocking | ||
| // forever. The goroutine may outlive the timeout since the underlying | ||
| // gRPC call cannot be cancelled from here, but it will be cleaned up | ||
| // when the connection is closed. | ||
| type arrowIterResult struct { | ||
| ai bigquery.ArrowIterator | ||
| err error | ||
| } | ||
| ch := make(chan arrowIterResult, 1) | ||
| go func() { | ||
| ai, err := iter.ArrowIterator() | ||
|
Contributor
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. I wonder if we need to fork to be able to pass a context down...we already had to fork to fix something else (Google has been unresponsive on the issue filed)
Author
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. idk. Should i TODO something here?
Contributor
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. @zeroshade is your Google friend up for fixing this too?
Contributor
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. Actually, this appears to be a plain getter: https://github.com/googleapis/google-cloud-go/blob/9773f607fd1d2d528cd82b2544fc10bce3c2ac74/bigquery/storage_iterator.go#L361-L377 Why do we need to wrap this in a timeout? |
||
| ch <- arrowIterResult{ai, err} | ||
| }() | ||
|
|
||
| storageCtx, cancel := context.WithTimeout(ctx, 30*time.Second) | ||
| defer cancel() | ||
|
|
||
| select { | ||
| case r := <-ch: | ||
| if r.err != nil { | ||
| return nil, -1, errToAdbcErr(adbc.StatusInternal, r.err, "read Arrow query results") | ||
| } | ||
| arrowIterator = r.ai | ||
| case <-storageCtx.Done(): | ||
| if ctx.Err() != nil { | ||
| // Parent context was cancelled by the caller — propagate it. | ||
| return nil, -1, adbc.Error{ | ||
| Code: adbc.StatusCancelled, | ||
| Msg: fmt.Sprintf("[bq] context cancelled while opening BigQuery Storage read session: %s", ctx.Err()), | ||
| } | ||
| } | ||
| // Our own 30s timeout fired — the Storage Read API is unreachable. | ||
| return nil, -1, adbc.Error{ | ||
| Code: adbc.StatusTimeout, | ||
| Msg: "[bq] BigQuery Storage Read API timed out after 30s. " + | ||
| "See " + OptionStringQueryBackendAPI + " (issue #66) for the future REST fallback.", | ||
| } | ||
| } | ||
| } else { | ||
| arrowIterator = emptyArrowIterator{iter.Schema} | ||
|
|
@@ -167,8 +220,8 @@ func getQueryParameter(values arrow.RecordBatch, row int, parameterMode string) | |
| return parameters, nil | ||
| } | ||
|
|
||
| func runPlainQuery(ctx context.Context, logger *slog.Logger, query *bigquery.Query, alloc memory.Allocator, resultRecordBufferSize int) (bigqueryRdr *reader, totalRows int64, err error) { | ||
| arrowIterator, totalRows, err := runQuery(ctx, logger, query, false) | ||
| func runPlainQuery(ctx context.Context, logger *slog.Logger, queryBackendAPI string, query *bigquery.Query, alloc memory.Allocator, resultRecordBufferSize int) (bigqueryRdr *reader, totalRows int64, err error) { | ||
| arrowIterator, totalRows, err := runQuery(ctx, logger, queryBackendAPI, query, false) | ||
| if err != nil { | ||
| return nil, -1, err | ||
| } | ||
|
|
@@ -213,7 +266,7 @@ func runPlainQuery(ctx context.Context, logger *slog.Logger, query *bigquery.Que | |
| return bigqueryRdr, totalRows, nil | ||
| } | ||
|
|
||
| func queryRecordWithSchemaCallback(ctx context.Context, logger *slog.Logger, group *errgroup.Group, query *bigquery.Query, rec arrow.RecordBatch, ch chan arrow.RecordBatch, parameterMode string, alloc memory.Allocator, rdrSchema func(schema *arrow.Schema)) (int64, error) { | ||
| func queryRecordWithSchemaCallback(ctx context.Context, logger *slog.Logger, queryBackendAPI string, group *errgroup.Group, query *bigquery.Query, rec arrow.RecordBatch, ch chan arrow.RecordBatch, parameterMode string, alloc memory.Allocator, rdrSchema func(schema *arrow.Schema)) (int64, error) { | ||
| totalRows := int64(-1) | ||
| for i := range int(rec.NumRows()) { | ||
| parameters, err := getQueryParameter(rec, i, parameterMode) | ||
|
|
@@ -224,7 +277,7 @@ func queryRecordWithSchemaCallback(ctx context.Context, logger *slog.Logger, gro | |
| query.Parameters = parameters | ||
| } | ||
|
|
||
| arrowIterator, rows, err := runQuery(ctx, logger, query, false) | ||
| arrowIterator, rows, err := runQuery(ctx, logger, queryBackendAPI, query, false) | ||
| if err != nil { | ||
| return -1, err | ||
| } | ||
|
|
@@ -249,9 +302,9 @@ func queryRecordWithSchemaCallback(ctx context.Context, logger *slog.Logger, gro | |
|
|
||
| // kicks off a goroutine for each endpoint and returns a reader which | ||
| // gathers all of the records as they come in. | ||
| func newRecordReader(ctx context.Context, logger *slog.Logger, query *bigquery.Query, boundParameters array.RecordReader, parameterMode string, alloc memory.Allocator, resultRecordBufferSize, prefetchConcurrency int) (bigqueryRdr *reader, totalRows int64, err error) { | ||
| func newRecordReader(ctx context.Context, logger *slog.Logger, queryBackendAPI string, query *bigquery.Query, boundParameters array.RecordReader, parameterMode string, alloc memory.Allocator, resultRecordBufferSize, prefetchConcurrency int) (bigqueryRdr *reader, totalRows int64, err error) { | ||
| if boundParameters == nil { | ||
| return runPlainQuery(ctx, logger, query, alloc, resultRecordBufferSize) | ||
| return runPlainQuery(ctx, logger, queryBackendAPI, query, alloc, resultRecordBufferSize) | ||
| } | ||
| defer boundParameters.Release() | ||
|
|
||
|
|
@@ -287,7 +340,7 @@ func newRecordReader(ctx context.Context, logger *slog.Logger, query *bigquery.Q | |
| // Each call to Record() on the record reader is allowed to release the previous record | ||
| // and since we're doing this sequentially | ||
| // we don't need to call rec.Retain() here and call call rec.Release() in queryRecordWithSchemaCallback | ||
| batchRows, err := queryRecordWithSchemaCallback(ctx, logger, group, query, rec, ch, parameterMode, alloc, func(schema *arrow.Schema) { | ||
| batchRows, err := queryRecordWithSchemaCallback(ctx, logger, queryBackendAPI, group, query, rec, ch, parameterMode, alloc, func(schema *arrow.Schema) { | ||
| bigqueryRdr.schema = schema | ||
| }) | ||
| if err != nil { | ||
|
|
||
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.
nit: we generally use the assert/require packages to clean up the assertions