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

aws_lambda_event_sources: Missing support for maxRecordAge=-1 value in KinesisEventSourceProps #27321

Closed
grollat opened this issue Sep 27, 2023 · 5 comments · Fixed by #29582
Closed
Labels
@aws-cdk/aws-lambda-event-sources docs/inline Related to inline documentation of the API Reference documentation This is a problem with documentation. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@grollat
Copy link

grollat commented Sep 27, 2023

Describe the bug

The service supports -1 as a value but CDK imposes a Duration object which does not support negative value. This can become an issue in case the Kinesis Stream Retention window is set to > 7 days (which is the max allowed for maxRecordAge). when this happens customer may lose records.

Expected Behavior

the KinesisEventSourceProps should support a way to set the maxRecordAge value to -1. (Duration(-1)?)

Current Behavior

Duration(-1) is not allowed and no other way to set this value

image

Reproduction Steps

self explanatory

Possible Solution

allow Duration.seconds(-1) as a way to pass through the value?

Additional Information/Context

No response

CDK CLI Version

latest

Framework Version

No response

Node.js Version

all

OS

all

Language

Typescript

Language Version

all

Other information

No response

@grollat grollat added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 27, 2023
@peterwoodworth peterwoodworth added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Sep 27, 2023
@peterwoodworth
Copy link
Contributor

Thanks for reporting, we may want to allow an override on Duration to allow this to work.

For now, you can work around this by overriding the template with escape hatches

@roger-zhangg
Copy link
Member

roger-zhangg commented Mar 12, 2024

Just FYI for who doesn't know, The default value for maxRecordAge is -1, document. So if you don't set this value at all, it will be automatically set to -1 in lambda.

E.g.:

const eventSource = new KinesisEventSource(stream, {
      startingPosition: lambda.StartingPosition.TRIM_HORIZON,
      tumblingWindow: cdk.Duration.seconds(60),
});

Image

@TheRealAmazonKendra
Copy link
Contributor

Thank you @roger-zhangg for your comment here. Since you can get the value of -1 by leaving this undefined, this seems like a documentation/doc string update is needed rather than a code change.

We should update the docstring here to reflect that not setting this value will result in -1. From the current docstring, though, it seems that it may also require not setting the field for the retention period as well (I'm not sure what that default is).

@TheRealAmazonKendra TheRealAmazonKendra added docs/inline Related to inline documentation of the API Reference documentation This is a problem with documentation. good first issue Related to contributions. See CONTRIBUTING.md and removed bug This issue is a bug. labels Mar 21, 2024
@roger-zhangg
Copy link
Member

roger-zhangg commented Mar 21, 2024

Thanks for the suggestion, I'll take a look at how retention period might affect the maxRecordAge.

Update:
It seems that the current docstring default @default - the retention period configured on the stream is actually saying that the default value of maxRecordAge in lambda event source mapping is -1. This setting make lambda never discard a record from Stream. And the retention period configured on the kinesis stream now decides the max record age. But for the maxRecordAge itself, it's still -1.

E.g

const fn = new TestFunction(this, 'F');
    const stream = new kinesis.Stream(this, 'Q', {retentionPeriod: cdk.Duration.days(7)});
    const eventSource = new KinesisEventSource(stream, {
      startingPosition: lambda.StartingPosition.TRIM_HORIZON,
      tumblingWindow: cdk.Duration.seconds(60),
    });

    fn.addEventSource(eventSource);

Output maxRecordAge is still -1. I'll go ahead to send a PR to make this docstring more understandable.

roger-zhangg added a commit to roger-zhangg/aws-cdk that referenced this issue Mar 22, 2024
roger-zhangg added a commit to roger-zhangg/aws-cdk that referenced this issue Mar 22, 2024
roger-zhangg added a commit to roger-zhangg/aws-cdk that referenced this issue Mar 22, 2024
roger-zhangg added a commit to roger-zhangg/aws-cdk that referenced this issue Mar 26, 2024
roger-zhangg added a commit to roger-zhangg/aws-cdk that referenced this issue Mar 26, 2024
mergify bot added a commit to roger-zhangg/aws-cdk that referenced this issue Mar 27, 2024
@mergify mergify bot closed this as completed in #29582 Mar 27, 2024
mergify bot pushed a commit that referenced this issue Mar 27, 2024
…ps (#29582)

### Issue # (if applicable)

Closes #27321

### Reason for this change

Our customer found `maxRecordAge`  can't be set to `-1` Due to limitation of `cdk.Duration`. However, it's not obvious in the docstring that the [default value](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-lambda-eventsourcemapping.html#cfn-lambda-eventsourcemapping-maximumrecordageinseconds) of `maxRecordAge` is already -1. Our customer can simply leave this prop empty to set it to -1. 

So, This PR improves the docstring to make default value more apparent.

See more discussion in the ticket.

### Description of changes

Improve docstring of `maxRecordAge` and `retryAttempts` in `KinesisEventSourceProps` in `aws-lambda-event-source`

### Description of how you validated changes



### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda-event-sources docs/inline Related to inline documentation of the API Reference documentation This is a problem with documentation. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
4 participants