Skip to content

Conversation

prateek-vats
Copy link
Contributor

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

Motivation and Context

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

Modifications

  • 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

Screenshots (if appropriate)

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

@millems
Copy link
Contributor

millems commented Aug 13, 2024

I'll be your PR reviewer! Once #5462 is shipped, feel free to update this PR with to fix any matching comments. Hopefully once that one is done, this one will go quick.

@prateek-vats
Copy link
Contributor Author

I'll be your PR reviewer! Once #5462 is shipped, feel free to update this PR with to fix any matching comments. Hopefully once that one is done, this one will go quick.

Sounds great! Thanks @millems

@prateek-vats prateek-vats force-pushed the consumed-capacity-transact-write-item branch 2 times, most recently from 11650bd to 9dded9f Compare August 17, 2024 20:08
@prateek-vats
Copy link
Contributor Author

I'll be your PR reviewer! Once #5462 is shipped, feel free to update this PR with to fix any matching comments. Hopefully once that one is done, this one will go quick.

@millems It looks like #5462 is merged. I've updated this PR as well

millems added a commit that referenced this pull request Aug 23, 2024
@millems
Copy link
Contributor

millems commented Aug 23, 2024

I had a few comments, but to help communicate them I just created a commit on top of yours for comparison: 4d5c584

I made these changes:

  1. A few typos that weren't yours.
  2. Removed the type parameters and items field from TransactWriteItemsEnhancedResponse. The type parameter was just being used for the items, and we never actually populated the items.
  3. Added a string getter/setter for enums that were missing them.
  4. Copy + make immutable the collections in TransactWriteItemsEnhancedResponse.

If you're cool with those changes, you can cherry-pick the commit and I'll give the ship-it tomorrow! Otherwise, let's talk about it.

@prateek-vats
Copy link
Contributor Author

I had a few comments, but to help communicate them I just created a commit on top of yours for comparison: 4d5c584

I made these changes:

  1. A few typos that weren't yours.
  2. Removed the type parameters and items field from TransactWriteItemsEnhancedResponse. The type parameter was just being used for the items, and we never actually populated the items.
  3. Added a string getter/setter for enums that were missing them.
  4. Copy + make immutable the collections in TransactWriteItemsEnhancedResponse.

If you're cool with those changes, you can cherry-pick the commit and I'll give the ship-it tomorrow! Otherwise, let's talk about it.

Thanks for making these changes. They look great to me.

I had a few comments, but to help communicate them I just created a commit on top of yours for comparison: 4d5c584

I made these changes:

  1. A few typos that weren't yours.
  2. Removed the type parameters and items field from TransactWriteItemsEnhancedResponse. The type parameter was just being used for the items, and we never actually populated the items.
  3. Added a string getter/setter for enums that were missing them.
  4. Copy + make immutable the collections in TransactWriteItemsEnhancedResponse.

If you're cool with those changes, you can cherry-pick the commit and I'll give the ship-it tomorrow! Otherwise, let's talk about it.

Thanks a lot, @millems , appreciate your help! I've pulled in your changes

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
65.4% Coverage on New Code (required ≥ 80%)
4.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@millems millems enabled auto-merge August 23, 2024 18:06
@millems millems added this pull request to the merge queue Aug 23, 2024
Merged via the queue into aws:master with commit b63ae7c Aug 23, 2024
0 of 9 checks passed
@millems
Copy link
Contributor

millems commented Aug 23, 2024

Thanks for your contribution! It will go out with the next regularly-schedule SDK release, some time next week.

@prateek-vats
Copy link
Contributor Author

Thanks for your contribution! It will go out with the next regularly-schedule SDK release, some time next week.

@millems I may have missed running the changelog script. Does that affect the release of these changes?

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