Skip to content

Conversation

@SanjayMarreddi
Copy link
Contributor

@SanjayMarreddi SanjayMarreddi commented Jun 18, 2025

Context
We have integrated analytics-accelerator-s3 into Iceberg in PR #12299. Currently, Iceberg customers need to enable the s3.analytics-accelerator.enabled flag in S3FileIOProperties to use the library. We plan to make this feature enabled by default in the future and have begun addressing the feature gaps between our current Analytics Accelerator Library (AAL) stream and the existing S3InputStream. This PR marks the beginning of our work toward default integration.

Description of PR
This PR modifies the existing integration tests to ensure compatibility when AAL is enabled by default. It incorporates all necessary async client setup code and skips integration tests that would currently fail if AAL is enabled in local setups. These skipped tests represent the feature gaps we are actively working to resolve.

Testing

  • All integration tests function properly with AAL disabled (current default behavior) and also work when AAL is enabled (with the exception of a few skipped tests).
  • Note that to enable AAL while running these tests, we just have to update this flag to true.

@SanjayMarreddi
Copy link
Contributor Author

@nastra @jackye1995 @geruh May I request for a review on this PR please?

PS: There are few other PRs ( #13348, #13361 ) lined up depending on this.

@SanjayMarreddi SanjayMarreddi force-pushed the integration_tests_with_analytics_accelerator_library branch from 7f1c064 to 878bee3 Compare June 24, 2025 14:01
@SanjayMarreddi SanjayMarreddi requested a review from nastra June 27, 2025 10:34
@SanjayMarreddi SanjayMarreddi force-pushed the integration_tests_with_analytics_accelerator_library branch from 878bee3 to 6277a3f Compare June 27, 2025 16:04
@nastra
Copy link
Contributor

nastra commented Jun 27, 2025

@nastra @jackye1995 @geruh May I request for a review on this PR please?

PS: There are few other PRs ( #13348, #13361 ) lined up depending on this.

I don't really have a way to run those tests, so it would be best if someone from AWS reviews them

@stubz151
Copy link
Contributor

hey @nastra I have run these tests as well and confirm they all working. Would like us to get an AWS maintainer to also look? or you happy with these changes?

@nastra nastra requested a review from amogh-jahagirdar June 30, 2025 12:56
@SanjayMarreddi SanjayMarreddi force-pushed the integration_tests_with_analytics_accelerator_library branch from 6277a3f to 344c918 Compare June 30, 2025 17:00
@SanjayMarreddi
Copy link
Contributor Author

SanjayMarreddi commented Jun 30, 2025

@nastra @jackye1995 @geruh May I request for a review on this PR please?
PS: There are few other PRs ( #13348, #13361 ) lined up depending on this.

I don't really have a way to run those tests, so it would be best if someone from AWS reviews them

Yeah, I understand the concern here. While someone from AWS reviews it, I thought of adding the instructions here again so it will help anybody to run the tests.

  1. Enable AAL by updating this flag to true.
  2. Once the corresponding bucket and aws environment variables are set like any other Iceberg integration tests, we just do ./gradlew :iceberg-aws:integrationTest

Please let me know if there are any questions.

@geruh
Copy link
Contributor

geruh commented Jul 1, 2025

Thanks for this, @SanjayMarreddi! I was able to run and validate the tests successfully. However, I’m a little concerned with the current approach, unless I'm missing something. Right now it seems we are manually setting the config and running the tests, and that could result in potential gaps.

Since this configuration introduces a dependency, it's different from the existing S3 related configs. To reduce the risk of missing bugs, I’d suggest either splitting the tests into two sets or adding some additional tests specifically for the analytic accelerator. Relying on the manual config setup during testing might cause issues to slip through and not be caught during releases. What do you think?

@stubz151
Copy link
Contributor

stubz151 commented Jul 1, 2025

That makes sense @geruh.
Was thinking I could set parameters at the start of the class and just run with both on/off. We happy with that?

@geruh
Copy link
Contributor

geruh commented Jul 1, 2025

Yes, that makes sense then we can always have a test for both paths for each release

@SanjayMarreddi SanjayMarreddi force-pushed the integration_tests_with_analytics_accelerator_library branch 3 times, most recently from 79218c1 to a462fd4 Compare July 3, 2025 17:26
@SanjayMarreddi
Copy link
Contributor Author

Hi @geruh, I have updated the PR with a new commit that enables running all the relevant integration tests with and without AAL as discussed above. Please have a look at it. Thanks!

@SanjayMarreddi SanjayMarreddi force-pushed the integration_tests_with_analytics_accelerator_library branch from a462fd4 to 44ac26e Compare July 4, 2025 12:54
@SanjayMarreddi SanjayMarreddi requested a review from stubz151 July 4, 2025 12:56
@SanjayMarreddi SanjayMarreddi force-pushed the integration_tests_with_analytics_accelerator_library branch from 44ac26e to 8999b0c Compare July 4, 2025 13:00
@SanjayMarreddi SanjayMarreddi force-pushed the integration_tests_with_analytics_accelerator_library branch from 8999b0c to 2631478 Compare July 8, 2025 12:59
@stubz151 stubz151 force-pushed the integration_tests_with_analytics_accelerator_library branch from 987124e to f05c461 Compare July 30, 2025 15:12
@stubz151 stubz151 force-pushed the integration_tests_with_analytics_accelerator_library branch from f05c461 to 27da641 Compare July 30, 2025 15:13
@github-actions
Copy link

github-actions bot commented Sep 7, 2025

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 Sep 7, 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 Sep 14, 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.

5 participants