getlift / lift

Expanding Serverless Framework beyond functions using the AWS CDK
MIT License
912 stars 111 forks source link

Add Max Concurrency configuration for AWS sqs. #299

Closed htxiong closed 1 year ago

htxiong commented 1 year ago

this is the PR for issue https://github.com/getlift/lift/issues/294

Use-case description

12 Jan 2023 AWS introduced maximum concurrency of AWS Lambda functions when using Amazon SQS as an event source (see release announcement here and compute blog from AWS)

And then Serverless started to support it in late Jan 2023.

Add the configuration for maximumConcurrency to the queue.

Example Config

    batchSize: 1
    maxRetries: 5
    maxConcurrency: 10
    worker:
htxiong commented 1 year ago

this is the PR for issue https://github.com/getlift/lift/issues/294

mnapoli commented 1 year ago

Could you update the documentation as well?

htxiong commented 1 year ago

@mnapoli addressed your comments. For the limit, AWS support only a number between 2 and 1000.

htxiong commented 1 year ago

@mnapoli @j0k3r @kamalgill @jakejscott can someone review this PR again?

mnapoli commented 1 year ago

@htxiong please don't ping users of the package unnecessarily.

It seems the tests are failing though.

htxiong commented 1 year ago

@htxiong please don't ping users of the package unnecessarily.

It seems the tests are failing though.

its weird, as it passed from my local. and i ran it again and it passed.

mnapoli commented 1 year ago

Maybe it's necessary to update the CDK dependency?

htxiong commented 1 year ago

i updated it to 2.65.0, it works as well. I do not know will it pass on CI if i push my change

htxiong commented 1 year ago
Screenshot 2023-02-21 at 9 19 52 pm

it all passed.

fredericbarthelet commented 1 year ago

Hi @htxiong, I updated dev dependencies in #308 Could you rebase and repush your PR, including conflict solving in package.json Hopefully, tests will be green and PR should be mergeable afterwards :)

Thanks

htxiong commented 1 year ago

thanks @fredericbarthelet, its done. wait for someone to trigger the build and see will it pass all tests.

htxiong commented 1 year ago

i merged the latest master and can someone trigger the CI job please?

mnapoli commented 1 year ago

Done, tests are failing with serverless v2.36.

Is there a way to mark this test as skipped for v2 (older) serverless versions?

htxiong commented 1 year ago

Done, tests are failing with serverless v2.36.

Is there a way to mark this test as skipped for v2 (older) serverless versions?

thanks, there is a way to add condition for tests, i will check how can i create a test condition for serverless versions.

htxiong commented 1 year ago

@mnapoli done, its not a nice way but it works as it.skipIf.

htxiong commented 1 year ago

@fredericbarthelet thanks and i changed it. please review it again and trigger the CI jobs.

fredericbarthelet commented 1 year ago

Thanks for the contribution @htxiong :)