Skip to content

Feature request: Leverage new DynamoDB Failed Conditional Writes behavior #1573

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

Closed
2 tasks done
brnkrygs opened this issue Jul 3, 2023 · 5 comments · Fixed by #1779
Closed
2 tasks done

Feature request: Leverage new DynamoDB Failed Conditional Writes behavior #1573

brnkrygs opened this issue Jul 3, 2023 · 5 comments · Fixed by #1779
Assignees
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility idempotency This item relates to the Idempotency Utility

Comments

@brnkrygs
Copy link
Contributor

brnkrygs commented Jul 3, 2023

Use case

The new idempotency utility uses conditional writes against DynamoDB for its locking mechanism.

When the conditional write fails, the record is considered locked and the request is already processed. In that case, a query is issued against DynamoDB to fetch the cached result and return it.

This means for this scenario that customers must wait and pay for both a write and read, even though the write fails.

On June 30, 2023, DynamoDB announced the ability for a failed conditional write to return a copy of the existing record. This would allow the package to skip performing the query, improving performance and lowering cost.

Solution/User Experience

In the idempotency utility, use the new DynamoDB conditional write behavior to return the current state of the item in the same conditional write operation. Remove the follow-up query.

Alternative solutions

No response

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

@brnkrygs brnkrygs added triage This item has not been triaged by a maintainer, please wait feature-request This item refers to a feature request for an existing or new utility labels Jul 3, 2023
@dreamorosi dreamorosi added idempotency This item relates to the Idempotency Utility discussing The issue needs to be discussed, elaborated, or refined and removed triage This item has not been triaged by a maintainer, please wait labels Jul 3, 2023
@dreamorosi dreamorosi added this to the Idempotency - GA Release milestone Jul 3, 2023
@dreamorosi
Copy link
Contributor

Hi @brnkrygs thank you for suggesting this improvement.

At least on the surface the proposal makes a lot of sense, before adding it to the backlog we'll have to investigate the feasibility based on the current request flow and understand if it fits as-is or we need rearchitecting.

We are going to start looking at this after we publish the first release.

@dreamorosi dreamorosi added the help-wanted We would really appreciate some support from community for this one label Jul 31, 2023
@dreamorosi dreamorosi removed this from the Idempotency - GA Release milestone Sep 13, 2023
@tolutheo
Copy link
Contributor

Taking a look at this issue as discussed with @dreamorosi

@tolutheo
Copy link
Contributor

Hi Andrea,

A couple of changes I am proposing to change in this feature request:

Implemented a custom error to contain existingRecord field here:

public constructor(message: string, existingRecord: IdempotencyRecord) {
    super(message);
    this.existingRecord = existingRecord;
  }

Error is then handled here using the code snippet below:

if (error instanceof ConditionalCheckFailedException) {
          if (!error.Item) {
            throw new Error('Item is undefined');
          }
          const Item = unmarshall(error.Item);
          throw new IdempotencyItemAlreadyExistsError(
            `Failed to put record for already existing idempotency key: ${record.idempotencyKey}`,
            new IdempotencyRecord({
              idempotencyKey: Item[this.keyAttr],
              status: Item[this.statusAttr],
              expiryTimestamp: Item[this.expiryAttr],
              inProgressExpiryTimestamp: Item[this.inProgressExpiryAttr],
              responseData: Item[this.dataAttr],
              payloadHash: Item[this.validationKeyAttr],
            })
          );
        }

To make use of the existingRecord field, it is referenced here:

const idempotencyRecord: IdempotencyRecord = e.existingRecord;

So the getRecord API call is not made again to fetch item that lead to failure of the conditional write, the code snippet below is removed from here:

const idempotencyRecord: IdempotencyRecord =
           await this.#persistenceStore.getRecord(
             this.#functionPayloadToBeHashed
           );

However the removal of that code causes a couple of unit tests listed below to fail with the error => The test expects getRecordSpy to be called once/twice/three times, but it is not called at all.:

  1. IdempotencyHandler.test.ts
  2. idempotencyDecorator.test.ts
  3. makeHandlerIdempotent.test.ts
  4. makeIdempotent.test.ts

To fix the issue we would need to remove the unit test getRecordSpy which is testing the getRecord code snippet mentioned above across those 4 unit test files.

For now I haven't removed the getRecord because the test files were failing and wanted to inform you about them first. Other changes made to unit test file here:

test('_putRecord throws Error when Item is undefined', async () => {
    // Prepare
    const persistenceLayer = new TestDynamoDBPersistenceLayer({
      tableName: dummyTableName,
    });
    const mockRecord = new IdempotencyRecord({
      idempotencyKey: 'test-key',
      status: 'INPROGRESS',
      expiryTimestamp: Date.now(),
    });

    DynamoDBClient.prototype.send = jest.fn().mockRejectedValueOnce(
      new ConditionalCheckFailedException({
        message: 'Conditional check failed',
        $metadata: {},
      })
    );
    await expect(persistenceLayer._putRecord(mockRecord)).rejects.toThrowError(
      'Item is undefined'
    );
  });

Copy link
Contributor

github-actions bot commented Jan 4, 2024

⚠️ 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.

@dreamorosi dreamorosi added completed This item is complete and has been merged/shipped and removed discussing The issue needs to be discussed, elaborated, or refined labels Jan 26, 2024
@dreamorosi dreamorosi moved this from Coming soon to Shipped in Powertools for AWS Lambda (TypeScript) Jan 26, 2024
@dreamorosi
Copy link
Contributor

This feature is available as of v1.8.0.

@dreamorosi dreamorosi removed the help-wanted We would really appreciate some support from community for this one label Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility idempotency This item relates to the Idempotency Utility
Projects
3 participants