Aws sdk migration#1845
Open
IvanBorislavovDimitrov wants to merge 5 commits into
Open
Conversation
IvanBorislavovDimitrov
force-pushed
the
aws-sdk-migration
branch
11 times, most recently
from
616a49d to
2a642fa
Compare
Yavor16
requested changes
Jun 3, 2026
Yavor16
left a comment
Yavor16
left a comment
Contributor
There was a problem hiding this comment.
The azure sdk files are also shown as deleted in this PR. Maybe if you rebase, it will remove from the PR
| <immutables.version>2.12.1</immutables.version> | ||
| <micrometer.version>1.16.4</micrometer.version> | ||
| <aliyun-sdk-oss.version>3.18.5</aliyun-sdk-oss.version> | ||
| <aws.sdk.version>2.44.12</aws.sdk.version> |
Contributor
There was a problem hiding this comment.
Before merge you can bump the version because they release new version very frequently
LMCROSSITXSADEPLOY-3315
Previously testConnection probed for a non-existent test object on the underlying store, which produced misleading results when the bucket or container itself was missing. The check now explicitly verifies that the target bucket/container exists and throws IllegalStateException with a clear OBJECT_STORE_BUCKET_NOT_FOUND message when it does not, making configuration errors fail fast with an actionable diagnostic. LMCROSSITXSADEPLOY-3315
IvanBorislavovDimitrov
force-pushed
the
aws-sdk-migration
branch
from
2a642fa to
c105981
Compare
IvanBorislavovDimitrov
force-pushed
the
aws-sdk-migration
branch
6 times, most recently
from
6489244 to
50c2cfc
Compare
IvanBorislavovDimitrov
force-pushed
the
aws-sdk-migration
branch
from
50c2cfc to
2c33caf
Compare
Yavor16
requested changes
Jun 9, 2026
IvanBorislavovDimitrov
force-pushed
the
aws-sdk-migration
branch
from
c629049 to
b2856e5
Compare
IvanBorislavovDimitrov
force-pushed
the
aws-sdk-migration
branch
from
b2856e5 to
78d335c
Compare
|
|
|
||
| private int deleteByFilterAndCount(BiPredicate<String, Map<String, String>> predicate) { | ||
| List<String> toDelete = new ArrayList<>(); | ||
| try (ExecutorService executor = Executors.newVirtualThreadPerTaskExecutor()) { |
Contributor
There was a problem hiding this comment.
Shouldn't we manage some central executor service at class level?
Contributor
Author
There was a problem hiding this comment.
In this case it is not needed, because the creation is cheap and no need to manage the lifecycle of the pool like shutdowning it. It is a virtual thread pool, the threads are not os threads.
| } | ||
| } | ||
|
|
||
| private static final class CredentialKeys { |
Contributor
There was a problem hiding this comment.
Why whole new class only with constants? Can we just keep constants in the same class or at least extract them into existing Constant class?
Contributor
Author
There was a problem hiding this comment.
So every SDK to manage the credentials it needs. It seems more readable for me, but if you think it is less readable can refactor it?
| } | ||
| } | ||
|
|
||
| // BlobStore::list may not return a complete list, making this method unreliable |
Contributor
Author
There was a problem hiding this comment.
I am not really sure, but the Jclouds will only be used for now for Ali cloud, and will be deleted after this migration so I guess its fine to leave it as is.
| public static final String DELETED_0_SECRET_TOKENS_WITH_EXPIRATION_DATE_1 = "Deleted secret tokens \"{0}\" with an expiration date \"{1}\""; | ||
| public static final String FAILED_TO_DELETE_FILE_0_IN_OBJECT_STORE_REASON_1 = "Failed to delete file \"{0}\" in object store. Reason: {1}"; | ||
| public static final String S3_UPLOAD_FAILED_FILE_0_SIZE_1 = "S3 upload failed for file \"{0}\" (size={1}). Root cause chain: {2}"; | ||
| public static final String BYTES_RANGE_0_1 = "bytes=%d-%d"; |
Contributor
There was a problem hiding this comment.
This is not exactly a message. I think this is for Constant class.
| if (hasJobStuck(existingJob)) { | ||
| LOGGER.warn(MessageFormat.format(Messages.JOB_WITH_ID_WAS_NOT_UPDATED_WITHIN_SECONDS_ON_START_0_1, existingJob.getId(), | ||
| UPDATE_JOB_TIMEOUT)); | ||
| LOGGER.warn(MessageFormat.format(Messages.STALE_JOB_DETAILS_0_1_2_3_4_5_6_7_8_9_10, existingJob.getId(), existingJob.getState(), |
Contributor
There was a problem hiding this comment.
Can we just add default method in AsyncUploadJobEntry class which returns build string and avoid formatting in place.
| LOGGER.info(Messages.JOB_WITH_ID_WAS_NOT_UPDATED_WITHIN_SECONDS, job.getId(), UPDATE_JOB_TIMEOUT); | ||
| LOGGER.warn(MessageFormat.format(Messages.JOB_WITH_ID_WAS_NOT_UPDATED_WITHIN_SECONDS_ON_GET_0_1, job.getId(), | ||
| UPDATE_JOB_TIMEOUT)); | ||
| LOGGER.warn(MessageFormat.format(Messages.STALE_JOB_DETAILS_0_1_2_3_4_5_6_7_8_9_10, job.getId(), job.getState(), |
Contributor
There was a problem hiding this comment.
Same can be applied here
| private static final Logger LOGGER = LoggerFactory.getLogger(AsyncUploadJobOrchestrator.class); | ||
|
|
||
| private final ResilientOperationExecutor resilientOperationExecutor = getResilientOperationExecutor(); | ||
| private final FileUploadResilientOperationExecutor fileUploadResilientOperationExecutor = getFileUploadResilientOperationExecutor(); |
Contributor
There was a problem hiding this comment.
What ResilientOperationExecutor can't do and it's necessary to add additional resilient executor?
| public static final String CONTAINER_URI = "container_uri"; | ||
| public static final String BASE_64_ENCODED_PRIVATE_KEY_DATA = "base64EncodedPrivateKeyData"; | ||
| public static final String HOST = "host"; | ||
| public static final Set<String> OBJECT_STORE_CUSTOM_REGIONS = Set.of("eu-south-1"); |
Contributor
There was a problem hiding this comment.
Why we need from custom regions?
| public static final String BASE_64_ENCODED_PRIVATE_KEY_DATA = "base64EncodedPrivateKeyData"; | ||
| public static final String HOST = "host"; | ||
| public static final Set<String> OBJECT_STORE_CUSTOM_REGIONS = Set.of("eu-south-1"); | ||
| public static final String OBJECT_STORE_JCLOUDS_REGIONS = "jclouds.regions"; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.