-
Notifications
You must be signed in to change notification settings - Fork 227
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
EDSC-3612, make search results export asynchronous #1588
Conversation
…sts__/handler.test.js
…-sqs, adding scripts for installing and running sqs and s3 simulations
…t S3 buckets, used by search exporting
…ss-configs/aws-resources.yml
…ocking of randomUUID
…onnections while running tests in parallel
…ng local development
@@ -130,6 +130,50 @@ | |||
environment: | |||
updateOrderStatusStateMachineArn: ${self:resources.Outputs.UpdateOrderStatusWorkflow.Value} | |||
|
|||
exportSearchRequest: | |||
handler: serverless/src/exportSearchRequest/handler.default | |||
timeout: 300 # this lambda isn't restricted to 30s because it's triggered by SQS not API Gateway |
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.
move comment down
exportSearchRequest: | ||
handler: serverless/src/exportSearchRequest/handler.default | ||
timeout: 300 # this lambda isn't restricted to 30s because it's triggered by SQS not API Gateway | ||
memorySize: 256 |
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.
keep memory default
Properties: | ||
QueueName: ${self:custom.siteName}-SearchExportQueue | ||
ReceiveMessageWaitTimeSeconds: 20 | ||
VisibilityTimeout: 300 |
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.
make larger than lambda timeout
@@ -31,8 +31,7 @@ const edlOptionalAuthorizer = async (event) => { | |||
if (!jwtToken || jwtToken === '') { | |||
const authOptionalPaths = [ | |||
'/autocomplete', | |||
'/opensearch/granules', | |||
'/collections/export' |
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.
put /collections/export back in
const MOCK_KEY = '00000000-0000-0000-0000-000000000000' // see /__mocks__/crypto.js | ||
|
||
// over-rides randomUUID to return the MOCK_KEY | ||
jest.mock('crypto') |
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.
remove, for uuid package
@@ -0,0 +1,124 @@ | |||
import { randomUUID } from 'crypto' // if we do 'node:crypto', jest.mock won't work |
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.
use older library
const earthdataEnvironment = determineEarthdataEnvironment(headers) | ||
|
||
const jwt = getJwtToken(event) | ||
if (!jwt) throw Error("missing jwt") |
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.
make jwt optional
|
||
const { id: userId } = getVerifiedJwtToken(jwt, earthdataEnvironment) | ||
|
||
if (!userId) throw Error("failed getting userId from jwt") |
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.
auth is optional now
variables | ||
} | ||
|
||
const key = randomUUID() |
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.
use uuid lib
|
||
const dbConnection = await getDbConnection() | ||
|
||
await dbConnection('exports').insert({ |
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.
mock
const searchExportQueueUrl = getSearchExportQueueUrl() | ||
console.log('searchExportQueueUrl:', searchExportQueueUrl) | ||
|
||
await sqs.sendMessage({ |
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.
mock it
QueueUrl: searchExportQueueUrl, | ||
MessageBody: messageBody | ||
}).promise() | ||
console.log('posted to search export queue') |
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.
prefix with lambda name, proper caps
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.
and requestId!
statusCode: 200, | ||
headers: { | ||
...defaultResponseHeaders, | ||
'jwt-token': jwt |
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.
optiona
@@ -1,125 +1,189 @@ | |||
import crypto from 'node:crypto' |
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.
uuid lib instead
* @param {Object} event Details about the HTTP request that it received | ||
*/ | ||
const exportSearch = async (event, context) => { | ||
// https://stackoverflow.com/questions/49347210/why-aws-lambda-keeps-timing-out-when-using-knex-js | ||
// eslint-disable-next-line no-param-reassign | ||
context.callbackWaitsForEmptyEventLoop = false | ||
|
||
const { body, headers } = event | ||
if (process.env.IS_OFFLINE || process.env.JEST_WORKER_ID) { |
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.
re-evaluate
if (!filename) throw new Error("missing filename") | ||
if (!key) throw new Error("missing key") | ||
if (!requestId) throw new Error("missing requestId") | ||
if (!userId) throw new Error("missing userId") |
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.
optional
|
||
const { data, requestId } = JSON.parse(body) | ||
const updateState = (state) => dbConnection('exports').where({ user_id: userId, key }).update({ state }) |
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.
userId optional
"s3": "EXTRA_CORS_ALLOWED_ORIGINS=http://localhost:5000 MOTO_ALLOW_NONEXISTENT_REGION=True moto_server -H 0.0.0.0", | ||
"s3:install": "brew install moto", | ||
"s3:reset": "curl -X POST http://localhost:5000/moto-api/reset", | ||
"sqs": "java -jar elasticmq-server-1.3.9.jar", |
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'd rather have sqs offline set up the same way as other projects our team manages, with the docker compose file.
I hate the idea of running extra processes in order to run EDSC locally. This PR for serverless offline sqs shows that it is possible to disable sqs offline. So we'd get the benefit of queues locally, but only if the dev wants them. I don't think that is happening, but it is easy enough to open a new PR that is up to date and install the forked version of the plugin
superseded by #1630 |
Overview
What is the feature?
Please summarize the feature or fix.
What is the Solution?
Summarize what you changed.
What areas of the application does this impact?
List impacted areas.
Testing
Reproduction steps
Attachments
Please include relevant screenshots or files that would be helpful in reviewing and verifying this change.
Checklist