-
Notifications
You must be signed in to change notification settings - Fork 492
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
fix(amazonq): retry S3 upload #6246
Changes from all commits
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,4 @@ | ||
{ | ||
"type": "Bug Fix", | ||
"description": "Amazon Q Code Transformation: retry project upload up to 3 times" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,18 +103,45 @@ export async function uploadArtifactToS3( | |
try { | ||
const uploadFileByteSize = (await nodefs.promises.stat(fileName)).size | ||
getLogger().info( | ||
`Uploading project artifact at %s with checksum %s using uploadId: %s and size %s kB`, | ||
`CodeTransformation: Uploading project artifact at %s with checksum %s using uploadId: %s and size %s kB`, | ||
fileName, | ||
sha256, | ||
resp.uploadId, | ||
Math.round(uploadFileByteSize / 1000) | ||
) | ||
|
||
const response = await request.fetch('PUT', resp.uploadUrl, { | ||
body: buffer, | ||
headers: getHeadersObj(sha256, resp.kmsKeyArn), | ||
}).response | ||
getLogger().info(`CodeTransformation: Status from S3 Upload = ${response.status}`) | ||
let response = undefined | ||
/* The existing S3 client has built-in retries but it requires the bucket name, so until | ||
* CreateUploadUrl can be modified to return the S3 bucket name, manually implement retries. | ||
* Alternatively, when waitUntil supports a fixed number of retries and retriableCodes, use that. | ||
*/ | ||
const retriableCodes = [408, 429, 500, 502, 503, 504] | ||
for (let i = 0; i < 4; i++) { | ||
try { | ||
response = await request.fetch('PUT', resp.uploadUrl, { | ||
body: buffer, | ||
headers: getHeadersObj(sha256, resp.kmsKeyArn), | ||
}).response | ||
getLogger().info(`CodeTransformation: upload to S3 status on attempt ${i + 1}/4 = ${response.status}`) | ||
if (response.status === 200) { | ||
break | ||
} | ||
throw new Error('Upload failed') | ||
} catch (e: any) { | ||
if (response && !retriableCodes.includes(response.status)) { | ||
throw new Error(`Upload failed with status code = ${response.status}; did not automatically retry`) | ||
} | ||
if (i !== 3) { | ||
await sleep(1000 * Math.pow(2, i)) | ||
} | ||
} | ||
} | ||
if (!response || response.status !== 200) { | ||
const uploadFailedError = `Upload failed after up to 4 attempts with status code = ${response?.status ?? 'unavailable'}` | ||
getLogger().error(`CodeTransformation: ${uploadFailedError}`) | ||
throw new Error(uploadFailedError) | ||
} | ||
getLogger().info('CodeTransformation: Upload to S3 succeeded') | ||
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'm not a big fan of implementing our own retry logic rather than doing it at the SDK layer, but the existing S3 client's upload functionality requires a bucket name as a part of the request, and 1) I don't think we should have the internal S3 bucket names we use floating around this repo, 2) we use multiple different buckets, and 3) we don't have access to the bucket name in the response of our 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. Thanks for that context! That info is probably worth a code comment here or in the docstring of this function. For reference, we have discussed some sort of util to "poll/retry stuff", see https://taskei.amazon.dev/tasks/IDE-15280 (startDevEnvironmentWithProgress is a very complex case, maybe not exactly analogous to standard "request retry"). |
||
} catch (e: any) { | ||
let errorMessage = `The upload failed due to: ${(e as Error).message}. For more information, see the [Amazon Q documentation](${CodeWhispererConstants.codeTransformTroubleshootUploadError})` | ||
if (errorMessage.includes('Request has expired')) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,8 @@ | |
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
import { BuildSystem, JDKVersion, TransformationCandidateProject } from '../../models/model' | ||
import { getLogger } from '../../../shared/logger' | ||
import * as CodeWhispererConstants from '../../models/constants' | ||
import { BuildSystem, TransformationCandidateProject } from '../../models/model' | ||
import * as vscode from 'vscode' | ||
// Consider using ChildProcess once we finalize all spawnSync calls | ||
import { spawnSync } from 'child_process' // eslint-disable-line no-restricted-imports | ||
import { | ||
NoJavaProjectsFoundError, | ||
NoMavenJavaProjectsFoundError, | ||
|
@@ -70,72 +66,9 @@ async function getMavenJavaProjects(javaProjects: TransformationCandidateProject | |
return mavenJavaProjects | ||
} | ||
|
||
async function getProjectsValidToTransform(mavenJavaProjects: TransformationCandidateProject[]) { | ||
const projectsValidToTransform: TransformationCandidateProject[] = [] | ||
for (const project of mavenJavaProjects) { | ||
let detectedJavaVersion = undefined | ||
const projectPath = project.path | ||
const compiledJavaFiles = await vscode.workspace.findFiles( | ||
new vscode.RelativePattern(projectPath!, '**/*.class'), | ||
'**/node_modules/**', | ||
1 | ||
) | ||
if (compiledJavaFiles.length > 0) { | ||
const classFilePath = `${compiledJavaFiles[0].fsPath}` | ||
const baseCommand = 'javap' | ||
const args = ['-v', classFilePath] | ||
const spawnResult = spawnSync(baseCommand, args, { shell: false, encoding: 'utf-8' }) | ||
if (spawnResult.status !== 0) { | ||
let errorLog = '' | ||
errorLog += spawnResult.error ? JSON.stringify(spawnResult.error) : '' | ||
errorLog += `${spawnResult.stderr}\n${spawnResult.stdout}` | ||
getLogger().error(`CodeTransformation: Error in running javap command = ${errorLog}`) | ||
let errorReason = '' | ||
if (spawnResult.stdout) { | ||
errorReason = 'JavapExecutionError' | ||
} else { | ||
errorReason = 'JavapSpawnError' | ||
} | ||
if (spawnResult.error) { | ||
const errorCode = (spawnResult.error as any).code ?? 'UNKNOWN' | ||
errorReason += `-${errorCode}` | ||
} | ||
getLogger().error( | ||
`CodeTransformation: Error in running javap command = ${errorReason}, log = ${errorLog}` | ||
) | ||
} else { | ||
const majorVersionIndex = spawnResult.stdout.indexOf('major version: ') | ||
const javaVersion = spawnResult.stdout.slice(majorVersionIndex + 15, majorVersionIndex + 17).trim() | ||
if (javaVersion === CodeWhispererConstants.JDK8VersionNumber) { | ||
detectedJavaVersion = JDKVersion.JDK8 | ||
} else if (javaVersion === CodeWhispererConstants.JDK11VersionNumber) { | ||
detectedJavaVersion = JDKVersion.JDK11 | ||
} else { | ||
detectedJavaVersion = JDKVersion.UNSUPPORTED | ||
} | ||
} | ||
} | ||
|
||
// detectedJavaVersion will be undefined if there are no .class files or if javap errors out, otherwise it will be JDK8, JDK11, or UNSUPPORTED | ||
project.JDKVersion = detectedJavaVersion | ||
projectsValidToTransform.push(project) | ||
} | ||
return projectsValidToTransform | ||
} | ||
|
||
/* | ||
* This function filters all open projects by first searching for a .java file and then searching for a pom.xml file in all projects. | ||
* It also tries to detect the Java version of each project by running "javap" on a .class file of each project. | ||
* As long as the project contains a .java file and a pom.xml file, the project is still considered valid for transformation, | ||
* and we allow the user to specify the Java version. | ||
*/ | ||
// This function filters all open projects by first searching for a .java file and then searching for a pom.xml file in all projects. | ||
export async function validateOpenProjects(projects: TransformationCandidateProject[]) { | ||
const javaProjects = await getJavaProjects(projects) | ||
|
||
const mavenJavaProjects = await getMavenJavaProjects(javaProjects) | ||
|
||
// These projects we know must contain a pom.xml and a .java file | ||
const projectsValidToTransform = await getProjectsValidToTransform(mavenJavaProjects) | ||
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. Deleted this function entirely because 1) it was expensive (running a shell command ( |
||
|
||
return projectsValidToTransform | ||
return mavenJavaProjects | ||
} |
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.
See comment below; rather than having 75+ LOC and running a shell command for each open project just to say "Java 8 project" or "Java 11 project" or "Java 8 and 11 projects", etc. just say "Java project"