Skip to content

feat: S3Client Cleanup and Error Handling Improvements#1149

Merged
ata-nas merged 13 commits intomainfrom
1118-s3-archive-plugin-cleanup-and-error-handling-improvements
Jun 3, 2025
Merged

feat: S3Client Cleanup and Error Handling Improvements#1149
ata-nas merged 13 commits intomainfrom
1118-s3-archive-plugin-cleanup-and-error-handling-improvements

Conversation

@ata-nas
Copy link
Contributor

@ata-nas ata-nas commented May 15, 2025

Reviewer Notes

  • some cleanups for the S3 client
  • I have removed the xml handler as it was creating some issues (or rather, was not flexible enough)
    • we cannot rely on Content-Lenght header to determine if there is a body or not unfortunately. It is not consistent.
    • some requests are not expected to have a body if successful, but will have if an error occurs. We need more flexibility to be able to dynamically parse or not a body.
  • currently tested with AWS S3, will test also with GCP
    • GCP should be compatible (request wise), the hope is that the same client can be reused (maybe authorization header will be different)

Related Issue(s)

Closes #1118
Closes #1074

@ata-nas ata-nas self-assigned this May 15, 2025
@ata-nas ata-nas added the pull request label to get past the "label required" check when no label is needed or appropriate. label May 15, 2025
@ata-nas ata-nas linked an issue May 15, 2025 that may be closed by this pull request
Copy link
Contributor Author

@ata-nas ata-nas 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 general questions. I have generally added some cleanup, a couple of new methods. I have tested with AWS S3, it is all working properly. GCP to follow. Collecting early feedback. Going back and forth on how to utilize the checked exceptions to do some error handling. Opted to remove the xml handler as it was causing issues: we cannot rely on the "Content-Length" header to determine if there is a body. Also the handler is not flexible enough for our needs, there are some requests that have no body if successful, but will have an xml body if the request fails (3xx+ response), and the handler will throw if no body or no xml body.

PLEASE NOTE: I have not yet gotten to testing the plugin itself. I have tested the client only. I will test the plugin itself as well, but will have to update with results. Also, more tests are to be added, have not gotten to that as well as of now.

@ata-nas ata-nas changed the title feat: S3 Archive Plugin Cleanup and Error Handling Improvements feat: S3Client Cleanup and Error Handling Improvements May 19, 2025
Copy link
Member

@georgi-l95 georgi-l95 left a comment

Choose a reason for hiding this comment

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

Did one round, will do another when tests are added. Solid progress 🙌

ata-nas added 4 commits May 28, 2025 11:08
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>

# Conflicts:
#	block-node/app/build.gradle.kts
#	settings.gradle.kts
@ata-nas ata-nas force-pushed the 1118-s3-archive-plugin-cleanup-and-error-handling-improvements branch from 2bf9425 to f21f162 Compare May 28, 2025 08:26
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
ata-nas added 2 commits May 29, 2025 15:09
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
@ata-nas ata-nas added this to the 0.12.0 milestone May 30, 2025
ata-nas added 4 commits June 2, 2025 14:11
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
…ecked exception

Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
@ata-nas ata-nas marked this pull request as ready for review June 2, 2025 13:12
@ata-nas ata-nas requested a review from a team as a code owner June 2, 2025 13:12
@ata-nas ata-nas requested a review from a team as a code owner June 2, 2025 13:12
@ata-nas
Copy link
Contributor Author

ata-nas commented Jun 2, 2025

UPDATE:
As we discussed with the team, let's proceed with what we have:

  1. The client is validated, working with AWS and GCP alike
  2. We have introduced basic error handling, but we will need to improve it
  3. We have done some cleanup and some test improvements

See the parent of the issue this PR closes (#1105) for other related issues that describe what needs to be done as a follow up.

AlfredoG87
AlfredoG87 previously approved these changes Jun 3, 2025
Copy link
Contributor

@AlfredoG87 AlfredoG87 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

jsync-swirlds
jsync-swirlds previously approved these changes Jun 3, 2025
Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>
@ata-nas ata-nas dismissed stale reviews from jsync-swirlds and AlfredoG87 via 669b787 June 3, 2025 17:59
Copy link
Contributor

@AlfredoG87 AlfredoG87 left a comment

Choose a reason for hiding this comment

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

LG!

@ata-nas ata-nas merged commit 3e5fb55 into main Jun 3, 2025
13 of 14 checks passed
@ata-nas ata-nas deleted the 1118-s3-archive-plugin-cleanup-and-error-handling-improvements branch June 3, 2025 18:35
@codecov
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

Attention: Patch coverage is 45.28302% with 145 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
.../hiero/block/node/base/s3/S3ResponseException.java 0.00% 72 Missing ⚠️
...in/java/org/hiero/block/node/base/s3/S3Client.java 67.79% 46 Missing and 11 partials ⚠️
...rg/hiero/block/node/base/s3/S3ClientException.java 0.00% 8 Missing ⚠️
.../node/base/s3/S3ClientInitializationException.java 0.00% 8 Missing ⚠️
@@             Coverage Diff              @@
##               main    #1149      +/-   ##
============================================
- Coverage     84.26%   82.11%   -2.16%     
- Complexity      915      917       +2     
============================================
  Files           105      107       +2     
  Lines          3743     3986     +243     
  Branches        413      427      +14     
============================================
+ Hits           3154     3273     +119     
- Misses          422      539     +117     
- Partials        167      174       +7     
Files with missing lines Coverage Δ Complexity Δ
.../org/hiero/block/common/utils/StringUtilities.java 100.00% <ø> (ø) 3.00 <0.00> (ø)
...rg/hiero/block/node/base/s3/S3ClientException.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../node/base/s3/S3ClientInitializationException.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...in/java/org/hiero/block/node/base/s3/S3Client.java 77.24% <67.79%> (-6.40%) 41.00 <24.00> (+4.00) ⬇️
.../hiero/block/node/base/s3/S3ResponseException.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pull request label to get past the "label required" check when no label is needed or appropriate.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S3Client Cleanup and Error Handling Improvements Validation of Cloud Archive S3

4 participants