Skip to content

Conversation

rishav-karanjit
Copy link
Member

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

github-actions bot commented Sep 8, 2025

Detected changes to the release files or to the check-files action

Copy link

github-actions bot commented Sep 8, 2025

Changes to the release files or the check-files action requires 2 approvals from CODEOWNERS

Copy link

github-actions bot commented Sep 8, 2025

Detected changes to the release files or to the check-files action

Copy link

github-actions bot commented Sep 8, 2025

Changes to the release files or the check-files action requires 2 approvals from CODEOWNERS

Copy link

github-actions bot commented Sep 9, 2025

Detected changes to the release files or to the check-files action

Copy link

github-actions bot commented Sep 9, 2025

Changes to the release files or the check-files action requires 2 approvals from CODEOWNERS

Copy link
Contributor

@texastony texastony left a comment

Choose a reason for hiding this comment

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

I think this a good start, but that the AI generated some very weird things that we do not need.
There are many un-needed imports and un-needed README entries.

/**
* Run a single batch put-get cycle and measure performance
*/
public Result runItemEncryptorCycle(final byte[] data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: This method appears to never read from nor write to a DynamoDB table.
But the README suggests that DDB Local is a requirement, and that a DDB table is required, as one is created.

Can we remove the DDB table creation and DDB local dependency?

- Local DynamoDB instance running on localhost:8000
- Access to AWS Database Encryption SDK for DynamoDB Java libraries

## Local DynamoDB Setup
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: I do not think Local DynamoDB is required.

Comment on lines +34 to +46
Create the test table:

```bash
aws dynamodb create-table \
--table-name db-esdk-performance-test \
--attribute-definitions \
AttributeName=partition_key,AttributeType=S \
AttributeName=sort_key,AttributeType=N \
--key-schema \
AttributeName=partition_key,KeyType=HASH \
AttributeName=sort_key,KeyType=RANGE \
--billing-mode PAY_PER_REQUEST \
--endpoint-url http://localhost:8000
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: I do not think this DDB table is required.

Comment on lines +164 to +167
### Batch Operations
- Performs BatchWriteItem operations with 25 items per batch
- Measures BatchGetItem operations with consistent reads
- Tests realistic DynamoDB workloads with encryption overhead
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: I did not see any DDB Batch operations... is the AI going wild?

### DB-ESDK Integration
- Uses AWS Database Encryption SDK for DynamoDB with transparent encryption
- Configures attribute actions (ENCRYPT_AND_SIGN, SIGN_ONLY, DO_NOTHING)
- Tests actual DynamoDB operations with client-side encryption
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: I did not see any actual DDB operations, just the Item Encryptor.


- Java 17 or higher
- Maven 3.6 or higher
- Local DynamoDB instance running on localhost:8000
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: I do not think that Local DynamoDB is required.

- Java 17 or higher
- Maven 3.6 or higher
- Local DynamoDB instance running on localhost:8000
- Access to AWS Database Encryption SDK for DynamoDB Java libraries
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 wording is very odd.

import me.tongfei.progressbar.ProgressBar;
import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration;
import software.amazon.awssdk.core.SdkBytes;
import software.amazon.awssdk.services.dynamodb.DynamoDbClient;
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: This is an un-used import... in fact... almost all of these DDB service imports are un-used... what is going on?
Why are there un-needed imports?

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.

2 participants