Aws sdk 2.x netty.nio - #210
Conversation
# Conflicts: # utils/hpc-client/hpc-cli-3.29.0.jar # utils/hpc-client/lib/hpc-common-3.29.0.jar # utils/hpc-client/lib/hpc-domain-types-3.29.0.jar # utils/hpc-client/lib/hpc-dto-3.29.0.jar
There was a problem hiding this comment.
🟡 Changes recommended
Locale-dependent client selection can reject valid configuration values, and IDE-only metadata should be removed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds configurable AWS SDK v2 S3 clients using CRT or Netty NIO, while retaining SDK v1 compatibility.
Changes:
- Adds CRT/Netty client implementations and selection factory.
- Updates AWS dependencies and S3 tuning.
- Extends Postman’s S3 download workflow.
File summaries
| File | Description |
|---|---|
src/hpc-server/pom.xml |
Updates AWS SDK versions. |
src/hpc-server/hpc-ws-rs-test/.../hpc-ws-rs-test-mock.xml |
Updates SDK v1 package references. |
src/hpc-server/hpc-ws-rs-impl/.../hpc-server.properties |
Adds client selection and tuning. |
src/hpc-server/hpc-scheduler/.../hpc-scheduler.properties |
Adds scheduler S3 configuration. |
src/hpc-server/hpc-scheduler-migration/.../hpc-scheduler-migration.properties |
Adds migration S3 configuration. |
src/hpc-server/hpc-integration-impl/.../hpc-integration-beans-configuration.xml |
Wires the SDK v2 factory and proxy. |
src/hpc-server/hpc-integration-impl/.../HpcS3ConnectionNettyNioAsyncClient.java |
Implements Netty NIO S3 connectivity. |
src/hpc-server/hpc-integration-impl/.../HpcS3ConnectionFactory.java |
Selects the configured async client. |
src/hpc-server/hpc-integration-impl/.../HpcS3ConnectionCrtAsyncClient.java |
Extracts CRT-specific behavior. |
src/hpc-server/hpc-integration-impl/.../v2/impl/HpcS3Connection.java |
Refactors shared S3 connection logic. |
src/hpc-server/hpc-integration-impl/.../v1/impl/HpcS3ProgressListener.java |
Moves the listener into the v1 package. |
src/hpc-server/hpc-integration-impl/.../v1/impl/HpcS3Connection.java |
Moves the connection into the v1 package. |
src/hpc-server/hpc-integration-impl/.../v1/impl/HpcDataTransferProxyImpl.java |
Moves the proxy into the v1 package. |
src/hpc-server/hpc-integration-impl/pom.xml |
Adds the Netty NIO dependency. |
src/hpc-server/hpc-dao-impl/hpc-dao-impl.iml |
Adds IDE-specific metadata. |
.../Sync Uplod/Get S3 Download Task Status.request.yaml |
Adds a download-status request. |
.../Sync Uplod/Download to AWS S3.request.yaml |
Captures task and destination identifiers. |
src/hpc-postman/.../.resources/definition.yaml |
Defines workflow variables. |
Review details
- Files reviewed: 18/23 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@copilot - add summary for this change |
There was a problem hiding this comment.
🟡 Changes recommended
The v2 proxy switch drops configured encryption and metadata validation, causing security and migration regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/hpc-postman/postman/collections/HPC Server API/Data Object Registration/Sync Uplod/Download to AWS S3.request.yaml:7
- When this request fails or returns no
taskId, the previous collection value is retained, so the next status request queries an unrelated stale task. Clear the variable before parsing each response, then populate it only when the current response supplies an ID.
src/hpc-server/hpc-integration-impl/src/main/resources/META-INF/spring/hpc-integration-beans-configuration.xml:65
- The newly activated v2 proxy does not override
validateDataObjectMetadata, so it inherits the interface default that throws “not supported.” S3 metadata-migration tasks call this method unconditionally, whereas the previous v1 proxy implements it; those tasks will now fail before completing. Implement the v2 validation method before enabling this bean, or retain the v1 proxy.
<bean id="hpcS3DataTransferProxy"
class="gov.nih.nci.hpc.integration.s3.v2.impl.HpcDataTransferProxyImpl" />
- Files reviewed: 17/22 changed files
- Comments generated: 1
- Review effort level: Balanced
Summary of the PR changes:
|
dinhys
left a comment
There was a problem hiding this comment.
Reviewed. The S3 Integration Properties document is useful to know the defaults!
No description provided.