test(gax): add showcase harness for resumable uploads - #9288
Conversation
Add the client-side implementation of the resumable upload protocol: - ResumableUploadDescriptor and ResumableUploadSession, plus the resumableUploadStub that generated clients wire into createApiCall - resumableSourceFromFile, a seekable source backed by a local file - CallOptions.resumableUpload carrying the transport context that generated clients pass to the stub - exports from index, fallback and descriptor, and client-libraries docs - unit and hermetic system tests covering the state machine, transient retries, recovery from state mismatches and resume from a saved URL The onProgress callback may return void; the documented usage logs progress without returning a value.
Adds an end-to-end harness for the gapic-showcase ResumableUploadService: a checked-in generated client, a sample that uploads a local file through a resumable session, and a run.sh that downloads the showcase server, compiles the client against the local google-gax checkout and runs the sample. Also documents the harness in test/README.md and ignores the protos it generates.
There was a problem hiding this comment.
Code Review
This pull request implements client-side support for the resumable upload protocol in google-gax, introducing the ResumableUploadSession state machine, file-backed seekable sources, and a showcase test harness. Feedback on the changes highlights an incorrect throughput constant (DEFAULT_UPLOAD_RATE_BYTES_PER_MS) that scales to gigabytes instead of megabytes per millisecond, as well as documentation examples in client-libraries.md that incorrectly reference uploadStream instead of uploadSource.
| export const DEFAULT_STALL_TIMEOUT_MS = 15 * 1000; | ||
| // Assumed sustained upload throughput, in bytes per millisecond, used to | ||
| // scale the global deadline when `uploadSize` is provided (~5 MB/s). | ||
| const DEFAULT_UPLOAD_RATE_BYTES_PER_MS = 5 * 1024 * 1024; |
There was a problem hiding this comment.
The value of DEFAULT_UPLOAD_RATE_BYTES_PER_MS seems to be incorrect based on the accompanying comment. The comment suggests an assumed throughput of ~5 MB/s, but the constant is defined as 5 * 1024 * 1024.
If this value is in bytes per millisecond as the variable name suggests, it equates to ~5 GB/s, which is likely far too high and will result in unreasonably short deadlines for large uploads.
For an assumed rate of 5 MB/s, the value in bytes per millisecond should be closer to 5 * 1024 * 1024 / 1000, which is approximately 5243. A value of 5 * 1024 (5120) would be a reasonable approximation.
| const DEFAULT_UPLOAD_RATE_BYTES_PER_MS = 5 * 1024 * 1024; | |
| const DEFAULT_UPLOAD_RATE_BYTES_PER_MS = 5 * 1024; |
| const helper = await client.createResumableUpload(request); | ||
| await helper.start({ | ||
| uploadStream: dataStream, | ||
| chunkSize: 8 * 1024 * 1024, // 8MB chunks | ||
| onProgress: status => { | ||
| console.log(`Committed ${status.bytesUploaded} bytes to ${status.uploadUrl}`); | ||
| }, | ||
| }); | ||
| const response = await helper.finished(); |
There was a problem hiding this comment.
The example for starting a resumable upload appears to be incorrect. The start method expects an uploadSource parameter of type ResumableSource, but the example uses uploadStream. Additionally, the dataStream variable is not defined, which could be confusing.
Note: Since this is an auto-generated file, please do not edit it directly as these changes will be overwritten. Instead, apply this fix upstream in the generator or templates.
| const helper = await client.createResumableUpload(request); | |
| await helper.start({ | |
| uploadStream: dataStream, | |
| chunkSize: 8 * 1024 * 1024, // 8MB chunks | |
| onProgress: status => { | |
| console.log(`Committed ${status.bytesUploaded} bytes to ${status.uploadUrl}`); | |
| }, | |
| }); | |
| const response = await helper.finished(); | |
| const uploadSource = client.getResumableSource(filePath); | |
| const helper = await client.createResumableUpload(request); | |
| await helper.start({ | |
| uploadSource, | |
| chunkSize: 8 * 1024 * 1024, // 8MB chunks | |
| onProgress: status => { | |
| console.log('Committed ' + status.bytesUploaded + ' bytes to ' + status.uploadUrl); | |
| }, | |
| }); | |
| const response = await helper.finished(); |
References
- Do not manually edit auto-generated files to fix typos or make other changes, as these edits will be overwritten during the next regeneration. Instead, apply the fixes upstream in the generator or templates.
| const helper = await client.createResumableUpload(); | ||
| await helper.start({ | ||
| uploadStream: dataStream, | ||
| resumeUrl: savedUploadUrl, | ||
| }); | ||
| const response = await helper.finished(); |
There was a problem hiding this comment.
Similar to the previous example, this code snippet for resuming an upload uses the incorrect uploadStream parameter. It should be uploadSource. When resuming, you still need to provide the same payload source.
Note: Since this is an auto-generated file, please do not edit it directly as these changes will be overwritten. Instead, apply this fix upstream in the generator or templates.
| const helper = await client.createResumableUpload(); | |
| await helper.start({ | |
| uploadStream: dataStream, | |
| resumeUrl: savedUploadUrl, | |
| }); | |
| const response = await helper.finished(); | |
| const uploadSource = client.getResumableSource(filePath); | |
| const helper = await client.createResumableUpload(); | |
| await helper.start({ | |
| uploadSource, | |
| resumeUrl: savedUploadUrl, | |
| }); | |
| const response = await helper.finished(); |
References
- Do not manually edit auto-generated files to fix typos or make other changes, as these edits will be overwritten during the next regeneration. Instead, apply the fixes upstream in the generator or templates.
The checked-in generated client is linted and type-checked as its own package, but CI installs the published google-gax (which does not have the resumable upload APIs yet) and never compiles protos/protos, so the client reported 12 type errors plus a promise/always-return error on every run. Treat it as a fixture instead: - move client/ to fixtures/, a path segment the monorepo linter ignores - rename its tsconfig.json to tsconfig.client.json, so the package detection walks up to google-gax's tsconfig and skips these files - update run.sh, sample.js, the client package.json and the harness README Verified by running the harness's own compile steps (compileProtos plus tsc -p tsconfig.client.json) against the local google-gax checkout.
Adds an end-to-end harness for the gapic-showcase
ResumableUploadService, so the resumable upload client path can be exercised against a real server. It uses the resumable upload support from thegoogle-gaxPR in this series, so merge that one first.google.showcase.v1beta1.ResumableUploadServicesample.js, which uploads a local file through a resumable sessionrun.sh, which downloads gapic-showcase, compiles the client against the localgoogle-gaxcheckout and runs the sampletest/README.md, and a.gitignoreentry for the generated protosThis is test tooling only; nothing in the published package changes.
Merge first: #9287
Related to: #9283