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

feature: Support of ReturnConsumedCapacity in DynamoDBEnhancedClient's BatchWrite operation #5462

Merged
merged 21 commits into from
Aug 17, 2024

Conversation

prateek-vats
Copy link
Contributor

@prateek-vats prateek-vats commented Aug 3, 2024

Motivation and Context

Currently the response returned by a BatchWriteItem and TransctWriteItem operations don't return the Capacity consumed unlike some other requests. This PR introduces that feature
Open issue: #4123

Modifications

  • Added CapacityConsumed to the BatchWriteResult POJO
  • Added support for returnCapacityConsumed to the EnhancedBatchWriteRequest
  • Make TransctWriteItems operation return TransctWriteEnhancedItemResponse which will contain ConsumedCapacity and ItemCollectionMetrics
  • Added support for returnCapacityConsumed to the TransctWriteEnhancedItemRequest

Testing

Added unit tests and functional tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@prateek-vats prateek-vats requested a review from a team as a code owner August 3, 2024 19:13
@prateek-vats prateek-vats changed the title feature: Add support for ConsumedCapacity in a BatchWrite request feature: Add support for ConsumedCapacity in a BatchWriteItems and TransactWriteItems request Aug 10, 2024
@prateek-vats prateek-vats changed the title feature: Add support for ConsumedCapacity in a BatchWriteItems and TransactWriteItems request feature: Support of ReturnConsumedCapacity in DynamoDBEnhancedClient's BatchWrite and TransctWriteItems operation Aug 10, 2024
@prateek-vats prateek-vats changed the title feature: Support of ReturnConsumedCapacity in DynamoDBEnhancedClient's BatchWrite and TransctWriteItems operation feature: Support of ReturnConsumedCapacity in DynamoDBEnhancedClient's BatchWrite operation Aug 12, 2024
@prateek-vats prateek-vats requested a review from zoewangg August 12, 2024 17:21
return BatchWriteResult.builder().unprocessedRequests(response.unprocessedItems()).build();
return BatchWriteResult.builder()
.unprocessedRequests(response.unprocessedItems())
.consumedCapacity(response.consumedCapacity())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: formatting seems a bit off.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -134,6 +138,16 @@ public void builder_missing_mapped_table_resource_error_message() {
errorMessageDeleteKeyFromItem);
}

@Test
public void hashCode_includesConsumedCapacity() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use equalsVerifier to test equals and hashcode? https://github.com/aws/aws-sdk-java-v2/blob/master/core/sdk-core/src/test/java/software/amazon/awssdk/core/async/ResponsePublisherTest.java#L25

We may need to add equalsverifier test dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test for equals and hashcode using equalsverifier. Looks like the test dependency was already there

@prateek-vats prateek-vats requested a review from zoewangg August 16, 2024 15:28
Copy link
Contributor

@zoewangg zoewangg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor comments on the javadoc. Triggering builds

}

/**
* The capacity units consumed by the {@code Write} operation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collection metrics?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad. Fixed

}

/**
* Set the capacity units consumed by the batch write operation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collection metrics?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad. Fixed

}

/**
* Whether to return the item collection metrics.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this method does not return a boolean value though. Suggesting

Sets the item collection metrics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

}

/**
* Whether to return the item collection metrics.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@prateek-vats prateek-vats requested a review from zoewangg August 16, 2024 17:33
@prateek-vats
Copy link
Contributor Author

prateek-vats commented Aug 16, 2024

Any ideas why all the jdk builds are failing?
@zoewangg

@zoewangg
Copy link
Contributor

It seems there are checkstyle erros

[ERROR] Failed to execute goal 
org.apache.maven.plugins:maven-checkstyle-plugin:3.1.2:check 
(checkstyle) on project dynamodb-enhanced: Failed during checkstyle 
execution: There are 6 errors reported by Checkstyle 8.42 with 
software/amazon/awssdk/checkstyle.xml ruleset. -> [Help 1]

Could you run it locally and fix them?

mvn clean install -pl :bom-internal,:dynamodb-enhanced -P quick --am
mvn clean install -pl :dynamodb-enhanced

@prateek-vats
Copy link
Contributor Author

It seems there are checkstyle erros

[ERROR] Failed to execute goal 
org.apache.maven.plugins:maven-checkstyle-plugin:3.1.2:check 
(checkstyle) on project dynamodb-enhanced: Failed during checkstyle 
execution: There are 6 errors reported by Checkstyle 8.42 with 
software/amazon/awssdk/checkstyle.xml ruleset. -> [Help 1]

Could you run it locally and fix them?

mvn clean install -pl :bom-internal,:dynamodb-enhanced -P quick --am
mvn clean install -pl :dynamodb-enhanced

Thank you, @zoewangg . I've made the changes to fix the build

Copy link

sonarcloud bot commented Aug 16, 2024

@zoewangg zoewangg added this pull request to the merge queue Aug 16, 2024
Merged via the queue into aws:master with commit b4ecd2d Aug 17, 2024
9 checks passed
@zoewangg
Copy link
Contributor

@all-contributors please add @prateek-vats for code

Copy link
Contributor

@zoewangg

I've put up a pull request to add @prateek-vats! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants