Conversation
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
84db439 to
bd6cd75
Compare
...ns-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3DataSegmentPullerTest.java
Fixed
Show fixed
Hide fixed
...ns-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3DataSegmentPullerTest.java
Fixed
Show fixed
Hide fixed
...ns-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3DataSegmentPullerTest.java
Fixed
Show fixed
Hide fixed
...sions-core/s3-extensions/src/test/java/org/apache/druid/data/input/s3/S3InputSourceTest.java
Fixed
Show fixed
Hide fixed
...sions-core/s3-extensions/src/test/java/org/apache/druid/data/input/s3/S3InputSourceTest.java
Fixed
Show fixed
Hide fixed
...-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/S3StorageConnector.java
Fixed
Show fixed
Hide fixed
...-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/S3StorageConnector.java
Fixed
Show fixed
Hide fixed
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentMover.java
Fixed
Show fixed
Hide fixed
...exing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisRecordSupplierTest.java
Fixed
Show fixed
Hide fixed
...exing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisRecordSupplierTest.java
Fixed
Show fixed
Hide fixed
3e2092e to
e2afbe4
Compare
cloud/aws-common/src/main/java/org/apache/druid/common/aws/AWSCredentialsUtils.java
Show resolved
Hide resolved
cloud/aws-common/src/test/java/org/apache/druid/common/aws/AWSClientUtilTest.java
Outdated
Show resolved
Hide resolved
...ons-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3ServerSideEncryption.java
Show resolved
Hide resolved
|
Very good job @jtuglu1! I have no objections, PR looks good from my perspective! |
|
👍 I'd still like to add 1-2 ITs that fully cover the newer S3 changes, but overall think it's basically there. |
e2afbe4 to
009f34e
Compare
|
Due to the criticality of the change, I think we should get 2 approvals for this PR. |
There was a problem hiding this comment.
Left a partial review.
Wanted to ask the following questions:
Did we test this patch on any cluster with the following things with we have s3 as the deep storage.
- Kinesis Ingestion.
- MSQ with Durable storage.
- MSQ select with durable storage as the destination.
- Segment getting nuked out.
- Task logs being viewable and getting purged out on S3.
- Segment ingestion.
Also have you tried this patch in any of the production clusters ?
cloud/aws-common/src/main/java/org/apache/druid/common/aws/FileSessionCredentialsProvider.java
Show resolved
Hide resolved
...-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/KinesisRecordSupplier.java
Outdated
Show resolved
Hide resolved
...s3-extensions/src/main/java/org/apache/druid/storage/s3/ObjectSummaryWithBucketIterator.java
Outdated
Show resolved
Hide resolved
...s3-extensions/src/main/java/org/apache/druid/storage/s3/ObjectSummaryWithBucketIterator.java
Show resolved
Hide resolved
|
Hi @cryptoe. Yes, I'm testing this on our production clusters running Kafka + S3. We don't have production clusters using Kinesis. I've only been able to run tests locally. I wonder if you folks might have some Kinesis clusters to test the new logic out on? |
12afa8d to
09135c5
Compare
09135c5 to
7a7a22d
Compare
7a7a22d to
aef9705
Compare
...rc/test/java/org/apache/druid/testing/embedded/docker/IngestionRollingUpgradeDockerTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/druid/testing/embedded/docker/IngestionRollingUpgradeDockerTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/druid/testing/embedded/docker/IngestionRollingUpgradeDockerTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/druid/testing/embedded/docker/IngestionRollingUpgradeDockerTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/druid/testing/embedded/docker/IngestionRollingUpgradeDockerTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/druid/testing/embedded/docker/IngestionRollingUpgradeDockerTest.java
Outdated
Show resolved
Hide resolved
4ae2e44 to
e9eb865
Compare
Closes #16903.
Description
Upgrade to AWS SDK v2.40.0. Looks like dependencies hdfs-storage + ranger have transitive dependencies on V1. Those will need to be excluded or upgraded as well. Core modules/extensions required for runtime are not reliant on V1.
Migrated to strictly synchronous APIs (to maintain backwards-compatibility with V1). A follow-up PR should be able to make use of async APIs where suitable. S3TransferManager is the only case where an async client may be used.
New Tests
IngestionRollingUpgradeDockerTestwhich simulates ingestion of Druid segments using differing Druid versions, verifying that indexing processes can talk to each other and brokers/historicals can read the segments.Hiccups
KinesisAggregatorV2not available in Maven – Maven via JitPack: amazon-kinesis-aggregator-2.0.2 has v1 code, amazon-kinesis-deaggregator-2.0.2 unavailable awslabs/kinesis-aggregation#120. Doesn't look like anything usesaggregation=true, so added a warning message. Seems like [FLINK-32097][Connectors/Kinesis] Implement support for Kinesis deaggregation flink-connector-aws#188 also ran into similar issues, but implemented their own unmerged approach. Per the warning on the https://github.com/awslabs/kinesis-aggregation repo, looks like data loss can occur anyways when using this mode: https://github.com/awslabs/kinesis-aggregation/blob/master/potential_data_loss.md so don't think that's really a desired feature anyways.Release note
Upgrade to AWS SDK v2.40.0
This PR has: