getlift / lift

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

Fix getlift/lift#208, allow for batchSize above 10 #317

Closed joawan closed 1 year ago

joawan commented 1 year ago

AWS allows for batchSize up to 10k for non-FIFO queues where you have also set a maxBatchWindow.

fredericbarthelet commented 1 year ago

Hi @joawan and thanks for the contribution :)

I was working on an exemple code snippet to showcase how to achieve the same functionality without relying on additional programmatic code within the Construct constructor, but relying solely on the JSON schema definition for the construct :

const QUEUE_DEFINITION = {
    type: "object",
    properties: {
        type: { const: "queue" },
        worker: {
            type: "object",
            properties: {
                timeout: { type: "number" },
            },
            additionalProperties: true,
        },
        maxRetries: { type: "number" },
        alarm: { type: "string" },
        batchSize: {
            type: "number",
            minimum: 1,
        },
        maxBatchingWindow: {
            type: "number",
            maximum: 300,
        },
        fifo: { type: "boolean" },
        delay: { type: "number" },
        encryption: { type: "string" },
        encryptionKey: { type: "string" },
    },
    oneOf: [
        {
            // For FIFO queues, the maximum batch size is 10
            properties: {
                fifo: { const: true },
                batchSize: {
                    type: "number",
                    maximum: 10,
                },
            },
        },
        {
            // When specifying a batch size larger than 10, maxBatchingWindow must be larger than 1
            properties: {
                batchSize: {
                    type: "number",
                    minimum: 11,
                    maximum: 10000,
                },
                maxBatchingWindow: {
                    type: "number",
                    minimum: 1,
                },
            },
            required: ["maxBatchingWindow"],
        },
    ],
    additionalProperties: false,
    required: ["worker"],
} as const;

This has the advantage to leverage existing validation functionalities without requiring additional cyclomatic complexity in the construct. WDYT ?

joawan commented 1 year ago

Looks way better!

fredericbarthelet commented 1 year ago

Hi @joawan, are you still up to update your PR with my recommendation :) ? I can do the update myself, but you have the original idea and deserve the credit adding this feature to Lift ;)

joawan commented 1 year ago

No no, your code, you update :)

joawan commented 1 year ago

Tried updating using your suggestion but then tests won't pass. Getting stuck on SQS visibility test

joawan commented 1 year ago

@mnapoli Any chance of getting this one in?

mnapoli commented 1 year ago

Let's get this shipped! Thanks for the PR!