Repository navigation
Conversation
google-cloud-bigquery 2.14.0 hardcodes the googleapis.com host for the request that opens a resumable upload session (HttpBigQueryRpc#open()), so BigqueryClient#load() used to take a test-only jobs.insert path under test_host, leaving the actual upload code untested. Add TestHostBigQueryRpc, an HttpBigQueryRpc subclass that builds that one URL from the configured host (what upstream does since googleapis/java-bigquery#2696), install it through the test_host BigQueryOptions, and drop the jobs.insert bypass. The two test-host classes move to their own package (testhost.TestHostBigQueryOptions / Rpc). The MockWebServer tests now see the real session-open POST and file PUT and assert on the uploaded bytes as well. loadAttemptResponses() / assertLoadAttempt() describe one load attempt (open, PUT, jobs.get) so retry scenarios list attempts instead of individual responses, and responses() flattens those arrays among single responses. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- Replace the Object-varargs responses() with the type-safe MockResponses builder (of/add/loadAttempt/toArray). - assertUploadLoadFile() now states that it models a single-chunk upload and fails with a clear message for files over the library's chunk size. - uploadedLines() detects gzip by its magic bytes instead of assuming it. - Update comments that still described the old jobs.insert load path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
630b219 to
b44c999
Compare
add(MockResponse...) already takes the array, so the shortcut only duplicated loadAttemptResponses(). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…g it Auto-detecting gzip would quietly accept a plain upload under compression: GZIP; stating the expectation turns it into a check. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A request for which no response was enqueued used to block until the client's read timeout (300 + 300 s with #113's defaults), so a test that listed one response too few hung for 10 minutes before failing. The MockWebServer now answers such requests immediately with a 599, and test_host.yml sets 1 s open/read timeouts so a server that deliberately stays silent (SocketPolicy.NO_RESPONSE) fails the test in a second as well. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…b ids Groundwork for tests of BigqueryClient#load()'s failure handling at the HTTP level: startResumableUploadResponse() is public and loadAttemptResponses() is split into uploadLoadFileResponse() / waitForLoadJobResponse(), so a test can put failing PUTs between the session open and the final chunk; disconnectDuringUploadResponse() is the one socket policy the library sees as a retryable network failure; uploadChunkAcceptedResponse(), assertUploadChunk() and writeLoadFile() cover files over the 15 MB chunk size; jobIdOf() reads the job id from the session-open POST. Tests pin what the library does on its own today: a 5xx or a connection reset during a chunk PUT is re-sent `retries` times on the same session, a 4xx is not, a 16 MB file goes up as two chunks, and a job that failed is retried under a new job id. An e2e run with compression: NONE checks the plain upload body. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- unansweredResponse() (SocketPolicy.NO_RESPONSE) and a test that a chunk PUT nobody answers fails after read_timeout_sec, retried once like any network failure: the timeout from #113 applies to the upload too. Costs about 2 s of real waiting. - A test of the harness itself: a request beyond the enqueued responses is answered with a 599 at once, and the run is reported as having made more requests than expected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
john-abre
left a comment
There was a problem hiding this comment.
レビューしました。test-host の継ぎ目自体は問題ないと思います。TestHostBigQueryRpc#open() は 2.14.0 の HttpBigQueryRpc#open()(パス、uploadType=resumable、ヘッダー、BigQueryException への変換)と同じ挙動で、write() も絶対 URL の Location を追っているので、モックサーバーへの配線は正しく動くはずです。
まだ approve はしていません。理由は以下の 2 点です。
1. マージ前に直してほしい: リクエストボディが最初の読み取りで消費される
uploadedLines()(BigqueryMockWebServerTestUtil.java:297)と既存の readRequestBodyAsString()(:173)はどちらも request.getBody().readByteArray() で RecordedRequest の Buffer を直接 drain しています。同じリクエストに対して 2 回目のボディ読み取りを行うと、空のバイト列が返ってきて JSONException: A JSONObject text must begin with { や GZIPInputStream の EOFException として失敗し、原因がハーネス側ではなくパーサー側にあるように見えてしまいます。
例: 同じセッション開始 POST に対して jobIdOf(requests.get(0)) の後に jobConfig(requests.get(0), "load") を呼ぶ、あるいは同じ PUT に対して uploadedLines() を 2 回呼ぶケース。#126 のカーソルで同じリクエストを複数回 assert しやすくなるので、今のうちに直しておきたいです。
修正は両方のヘルパーで getBody().clone() を読むだけです(:297 にインラインで suggestion を付けています)。:173 も同様に:
byte[] rawBytes = request.getBody().clone().readByteArray();2. approve の前提: load() の途中切れアップロード(#123 で対応する想定)
BigqueryClient.java:413 の load() は Files.copy() の IOException を catch して INFO ログだけ出し、そのまま try-with-resources が TableDataWriteChannel を close します。close 時にバッファ済みの内容が最終チャンクとして PUT されるので、ローカルの読み取りがファイル途中で失敗すると、途中までのファイルから load job が作られ、行が静かに失われます(flush 前に失敗した場合は getJob() が null になり、waitForLoad(null) が NPE になります)。
これは既存の挙動で、PR の説明でも #123 に委ねられているので、この PR で直す必要はないと思っています。ただし approve は「#123 がこの経路(catch で例外を握りつぶしている点と、close が部分ファイルをアップロードしてしまう点)を明示的にカバーする」ことを前提にしています。#123 のスコープがもっと狭い場合は、別 issue として切り出しておいてもらえると助かります。resumable upload がこの PR で唯一の load 経路になるので、忘れたくない箇所です。
その他(非ブロッキング、メモとして)
TestHostBigQueryRpcがライブラリの private な内部実装を再現している(TestHostBigQueryRpc.java:34): 2 つ目のHttpTransportとリクエストファクトリ、ハードコードされたアップロードパス、upstream のX-Upload-Content-Valueというヘッダーのタイポまで含めて。これは googleapis/java-bigquery#2696 で upstream 側が修正済みのバグへの回避策なので、google-cloud-bigquery をその修正を含むバージョンに上げればtesthostパッケージごと削除できます。今後のバージョンアップでHttpBigQueryRpcのコンストラクタやopen()/write()の契約が変わると、公開 API 的にはコンパイルが通ったまま静かに挙動がずれる可能性があります。バージョンを固定する理由があるなら、その理由をオーバーライドの横に記録しておいてほしいです。- モックサーバーの origin を 2 通りの式で組み立てている(
BigqueryMockWebServerTestUtil.java:85): resumable upload の Location は"http://" + server.getHostName() + ":" + server.getPort()で手組みし、クライアントのtest_hostはserver.url("/")から設定しています。TLS 化や IPv6 のホスト名表記などで食い違うと、チャンク PUT が別 origin に飛んで分かりにくい接続エラーになります。server.url("/")一本に寄せるのが良さそうです。 - spotless がインラインコメントを崩している(
TestBigqueryJavaOutputPluginWithMockServer.java:389、testRunReplaceModeStillUpdatesSchemaWhenTempTableDeleteFails):storeCachedSrcFieldsIfNeed()のコメントが 2 行に割れて)が浮いており、// delete temp table (finally)が本来のerrorResponse(400, ...)ではなく.toArray());に付いています。他のテストと同じく、説明対象のレスポンスの上の行に独立したコメントとして置くとフォーマッタに動かされません。 - ハーネスのセルフテストが JUnit 内部のメッセージ形式に依存している(
TestBigqueryMockWebServerTestUtil.java:47):expected:<1> but was:<2>という文言を assert しているので、JUnit のバージョンアップやrunWithMockServer()側のメッセージ変更でハーネス自体は壊れていないのにテストが落ちます。runWithMockServer()の件数 assert に独自メッセージ(例:unexpected request count)を付けてそれにマッチさせるか、server.getRequestCount()を直接 assert する形が安全です。 - リトライの説明コメントが
newLoadFile()に付いている(TestBigqueryClientWithMockServer.java:405): 「1 回の load attempt は 3 リクエストで、リトライごとに新しい job として再アップロードされる」という 3 行のコメントが、1 行のファイルを書くだけのヘルパーに付いています。testLoadRetriesOnInternalErrorThenSucceeds/testLoadGivesUpAfterMaxRetries(またはすでに 3 リクエスト構成を説明している util 側のloadAttemptResponses())に移したほうが読み手に届きます。
確認済み: test_host.yml の 2 秒タイムアウトは validateTimeouts を通ります(0 も許容)。PR head のツリーに、削除された BigqueryTestHostSupport、createLoadJobResponse、旧 1 引数の waitForLoadJobResponse への参照は残っていません。
🤖 Generated with Claude Code
| // (true for compression: GZIP, test_host.yml's default), so a plain body fails to gunzip. | ||
| public static List<String> uploadedLines(RecordedRequest request, boolean gzipped) | ||
| throws IOException { | ||
| byte[] body = request.getBody().readByteArray(); |
There was a problem hiding this comment.
RecordedRequest の Buffer を直接 drain すると、同じリクエストの 2 回目の読み取りが空になります。clone() を読めば何度でも読み直せます(readRequestBodyAsString() の :173 も同様に)。
| byte[] body = request.getBody().readByteArray(); | |
| byte[] body = request.getBody().clone().readByteArray(); |
|
This merge has been undone: the PR was squash-merged before the review here had been addressed and approved, so |
Summary
google-cloud-bigquery 2.14.0 hardcodes the
googleapis.comhost for the request that opens a resumable upload session (HttpBigQueryRpc#open(), v2.14.0#L723-L739). Undertest_host,BigqueryClient#load()therefore took a test-onlyjobs.insertpath, and the MockWebServer tests never exercised the actual upload code.TestHostBigQueryRpc, anHttpBigQueryRpcsubclass that overrides onlyopen()to build that URL from the configured host, as upstream does since fix: custom host for resumable uploads googleapis/java-bigquery#2696. It is installed as theServiceRpcFactoryof thetest_hostBigQueryOptions, so it never runs in production. The chunkPUTs and the library's own retries are the real code path.jobs.insertbypass fromload(). The test-host classes move to their own package:testhost.TestHostBigQueryOptions(wasBigqueryTestHostSupport) andtesthost.TestHostBigQueryRpc. Retry settings are no longer set there; after [ruby-compat/02] Apply retries/timeout config to every BigQuery API call, not just jobs #113 they come from the plugin config throughbuildRetrySettings(), like in production (test_host.ymlkeeps the backoff near zero).POSTand the filePUT, and the append_direct runs also check the uploaded bytes (gzipped JSONL, and plain JSONL undercompression: NONE).Test harness
test_host.ymlsets 1 s open/read timeouts, andrunWithMockServer()answers a request with no enqueued response with a 599 at once, so a test that lists one response too few (or a server that deliberately stays silent) fails in seconds instead of blocking until the production read timeout (300 + 300 s).startResumableUploadResponse(),uploadLoadFileResponse(jobId),waitForLoadJobResponse(jobId, errorReason), bundled byloadAttemptResponses()and checked byassertLoadAttempt().MockResponses.of(...).add(...).toArray()puts those arrays among single responses.disconnectDuringUploadResponse()(the one socket policy the library sees as a retryable network failure; the comment says why the others don't work),unansweredResponse()(read timeout), and plainerrorResponse()on aPUT.jobIdOf()reads the job id from the session-openPOST, to tell a re-upload under the same job from a new job.writeLoadFile()makes a file over the 15 MB chunk size,uploadChunkAcceptedResponse()answers an intermediate chunk,assertUploadChunk()checksContent-Range;assertUploadLoadFile()is the single-chunk case and says so when a file is larger.uploadedLines(request, gzipped)takes the expected compression instead of sniffing it.Tests pinning what the library does on its own today
PUTanswered with 5xx, a connection reset while sending, or no answer at all is re-sentretriestimes on the same session; a 4xx is not. (testLoadResendsChunkOnServerErrorThenGivesUp,testLoadResendsChunkOnConnectionReset,testLoadFailsOnReadTimeoutDuringUpload,testLoadDoesNotResendChunkOnClientError)testLoadUploadsLargeFileInChunks, about 200 ms).testLoadRetriesOnInternalErrorThenSucceeds).TestBigqueryMockWebServerTestUtil).Test plan
./gradlew test(about 16 s of test time; the read-timeout test waits 2 s)./gradlew spotlessApplyNotes
writeToStream()overridden) and log assertions.requests.get(n)indices with a sequential cursor is [ruby-compat] Assert recorded requests through a cursor instead of by index #126, stacked on this PR.🤖 Generated with Claude Code