Skip to content
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

Closed
wants to merge 44 commits into from
Closed

Conversation

DanielJDufour
Copy link
Contributor

@DanielJDufour DanielJDufour commented Feb 13, 2023

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

  • Environment for testing:
  • Collection to test with:
  1. Step 1
  2. Step 2...

Attachments

Please include relevant screenshots or files that would be helpful in reviewing and verifying this change.

Checklist

  • I have added automated tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

…-sqs, adding scripts for installing and running sqs and s3 simulations
@DanielJDufour DanielJDufour changed the title Work in Progress: EDSC-3612, make search results export asynchronous EDSC-3612, make search results export asynchronous Feb 27, 2023
@DanielJDufour DanielJDufour marked this pull request as ready for review February 27, 2023 14:38
@@ -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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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'
Copy link
Contributor Author

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')
Copy link
Contributor Author

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
Copy link
Contributor Author

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")
Copy link
Contributor Author

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")
Copy link
Contributor Author

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()
Copy link
Contributor Author

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({
Copy link
Contributor Author

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({
Copy link
Contributor Author

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')
Copy link
Contributor Author

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

Copy link
Contributor Author

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
Copy link
Contributor Author

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'
Copy link
Contributor Author

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) {
Copy link
Contributor Author

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")
Copy link
Contributor Author

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 })
Copy link
Contributor Author

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",
Copy link
Contributor

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

@DanielJDufour
Copy link
Contributor Author

superseded by #1630

@macrouch macrouch deleted the EDSC-3612 branch June 28, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants