EdgarModesto23 commented on code in PR #3045:
URL: https://github.com/apache/iggy/pull/3045#discussion_r3012929390
##########
core/connectors/sinks/iceberg_sink/src/props.rs:
##########
@@ -30,14 +30,49 @@ pub fn init_props(config: &IcebergSinkConfig) ->
Result<HashMap<String, String>,
fn get_props_s3(config: &IcebergSinkConfig) -> Result<HashMap<String, String>,
Error> {
let mut props: HashMap<String, String> = HashMap::new();
props.insert("s3.region".to_string(), config.store_region.clone());
- props.insert(
- "s3.access-key-id".to_string(),
- config.store_access_key_id.clone(),
- );
- props.insert(
- "s3.secret-access-key".to_string(),
- config.store_secret_access_key.clone(),
- );
+ if let Some(access_key_id) = &config.store_access_key_id {
Review Comment:
Did you manage to test end-to-end if the rest catalog falls back to the
default credential manager if the access-key and secret are not present? If so
please add the results in your PR. You can use this docker-compose to have a
basic minio/spark-iceberg deployment locally that you can test on.
```yaml
services:
spark-iceberg:
image: tabulario/spark-iceberg
container_name: spark-iceberg
build: spark/
networks:
iceberg_net:
depends_on:
- rest
- minio
volumes:
- ./warehouse:/home/iceberg/warehouse
- ./notebooks:/home/iceberg/notebooks/notebooks
environment:
- AWS_ACCESS_KEY_ID=admin
- AWS_SECRET_ACCESS_KEY=password
- AWS_REGION=us-east-1
ports:
- 8888:8888
- 8080:8080
- 10000:10000
- 10001:10001
rest:
image: apache/iceberg-rest-fixture
container_name: iceberg-rest
networks:
iceberg_net:
ports:
- 8181:8181
environment:
- AWS_ACCESS_KEY_ID=admin
- AWS_SECRET_ACCESS_KEY=password
- AWS_REGION=us-east-1
- CATALOG_WAREHOUSE=s3://warehouse/
- CATALOG_IO__IMPL=org.apache.iceberg.aws.s3.S3FileIO
- CATALOG_S3_ENDPOINT=http://minio:9000
minio:
image: minio/minio
container_name: minio
environment:
- MINIO_ROOT_USER=admin
- MINIO_ROOT_PASSWORD=password
- MINIO_DOMAIN=minio
networks:
iceberg_net:
aliases:
- warehouse.minio
ports:
- 9001:9001
- 9000:9000
command: ["server", "/data", "--console-address", ":9001"]
mc:
depends_on:
- minio
image: minio/mc
container_name: mc
networks:
iceberg_net:
environment:
- AWS_ACCESS_KEY_ID=admin
- AWS_SECRET_ACCESS_KEY=password
- AWS_REGION=us-east-1
entrypoint: |
/bin/sh -c "
until (/usr/bin/mc alias set minio http://minio:9000 admin password)
do echo '...waiting...' && sleep 1; done;
/usr/bin/mc rm -r --force minio/warehouse;
/usr/bin/mc mb minio/warehouse;
/usr/bin/mc policy set public minio/warehouse;
tail -f /dev/null
"
networks:
iceberg_net:
```
##########
core/connectors/sinks/iceberg_sink/src/sink.rs:
##########
@@ -30,22 +30,22 @@ use tracing::{debug, error, info};
#[async_trait]
impl Sink for IcebergSink {
async fn open(&mut self) -> Result<(), Error> {
- let redacted_store_key = self
- .config
- .store_access_key_id
- .chars()
- .take(3)
- .collect::<String>();
- let redacted_store_secret = self
- .config
- .store_secret_access_key
- .chars()
- .take(3)
- .collect::<String>();
- info!(
- "Opened Iceberg sink connector with ID: {} for URL: {}, store
access key ID: {redacted_store_key}*** store secret:
{redacted_store_secret}***",
- self.id, self.config.uri
- );
+ if let (Some(store_access_key_id), Some(store_secret_access_key)) = (
+ &self.config.store_access_key_id,
Review Comment:
If these are optional, maybe It'll be a good to fail if only one of them is
present so we can fail early.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
