-
Notifications
You must be signed in to change notification settings - Fork 0
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
Origin/idempotency core #5
Conversation
…d overarching error scenario
… add logic to call the idempotency hander
public constructor(private functiontoMakeIdempotent: AnyFunctionWithRecord<U>, private functionPayloadToBeHashed: unknown, | ||
private idempotencyOptions: IdempotencyOptions, private fullFunctionPayload: Record<string, any>) {} | ||
|
||
public determineResultFromIdempotencyRecord(idempotencyRecord: IdempotencyRecord): Promise<U> | U{ |
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.
This might be work for another PR, but we need to add comments to the public-facing methods that document how we expect the methods to be used
private idempotencyOptions: IdempotencyOptions, private fullFunctionPayload: Record<string, any>) {} | ||
|
||
public determineResultFromIdempotencyRecord(idempotencyRecord: IdempotencyRecord): Promise<U> | U{ | ||
if (idempotencyRecord.getStatus() === IdempotencyRecordStatus.EXPIRED) { |
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 notice the Python version has comments explaining why this is an inconsistent state. It may be helpful to add similar comments here for context.
throw new IdempotencyAlreadyInProgressError(`There is already an execution in progress with idempotency key: ${idempotencyRecord.idempotencyKey}`); | ||
} else { | ||
// Currently recalling the method as this fulfills FR1. FR3 will address using the previously stored value https://github.com/awslabs/aws-lambda-powertools-typescript/issues/447 | ||
return this.functiontoMakeIdempotent(this.fullFunctionPayload); |
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.
Does this achieve idempotency (FR1) or does it just prevent concurrent calls from executing the same code?
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.
The main question I have is: should we recall the method or should we just not return a response. I don't recall whether we discussed this earlier.
|
||
export { | ||
AnyFunction | ||
// AnyFunction, |
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.
Probably should be removed in the future.
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.
Still curious about the answers to my question, but nothing here should stop it in my opinion
Description of your changes
How to verify this change
Related issues, RFCs
Issue number:
PR status
Is this ready for review?: NO
Is it a breaking change?: NO
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.