Skip to content

Conversation

@stubz151
Copy link
Contributor

@stubz151 stubz151 commented Oct 3, 2025

What Am I doing

Removing the async client which AAL added in pr's:

reverting the majority of:
#12553
#12299

I am doing this because after testing/benchmarking and speaking with the teams involved, this is not a suitable use-case for the s3 async client so I am removing it.

I am also implementing the range readable interface on the AAL adapter to make better use of the new api's we have added.

How have I tested it

  • benchmarked
  • unit tests
  • integration tests

@github-actions github-actions bot added the AWS label Oct 3, 2025
@stubz151 stubz151 force-pushed the remove_async_client branch from 004940f to 83d28de Compare October 3, 2025 14:22
Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

Thank you for the change @stubz151

Since these are removing public interfaces directly, wondering if we should mark them deprecated first ? and set a version where we want them to get removed.

Also curious, if you can share benchmarks you sighted above.

@fuatbasik
Copy link

fuatbasik commented Oct 7, 2025

Hi @singhpk234. Thanks a lot for your comments. @stubz151 is out of office but I can answer your questions until he is back.

Since these are removing public interfaces directly, wondering if we should mark them deprecated first ? and set a version where we want them to get removed.

AFAIK these interfaces currently only used by AnalyticsStream. They were added when we initially added the stream but during the community sync at 8/27 there were concerns about the developer experience. If you still think we should deprecate them first, happy to do it.

Also curious, if you can share benchmarks you sighted above.

We recently added sync client to our microbenchmarks: https://github.com/awslabs/analytics-accelerator-s3/tree/main/input-stream/src/jmh and havent seen much difference between sync and async client. This is mostly because Analytics Accelerator already handles async logic itself, so no need for additional client complexity. It will still support Async client though, for users who are already in Async client. But as initially we introduced Async client. Following aforementioned community feedback about developer experince, we though we can remove Async client integration from Iceberg.

@stubz151 stubz151 changed the title AWS: removing s3 async client and adding aal vector path AWS: swapping s3 aal to the sync client and adding aal vector path Oct 17, 2025
@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 17, 2025
@github-actions
Copy link

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants