Repository navigation
refactor(glue): derive catalog configuration from properties - #3102
Conversation
196a1a1 to
29a45cf
Compare
Use the shared property derive for Glue catalog settings while preserving warehouse validation and forwarding AWS and storage properties. Tracks [apache#3096](apache#3096). Generated-by: Codex
29a45cf to
9612813
Compare
|
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 dev@iceberg.apache.org list. Thank you for your contributions. |
|
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. |
Derive Default for GlueCatalogBuilder, use the conventional tests module name, and remove the FileIO forwarding test requested in review. Generated-by: Codex
There was a problem hiding this comment.
load now hands the full map to the KMS factory and FileIO, uri/catalog_id/warehouse included. That's fine with me since it's the stated intent, but I'd like a test that pins it (left a note inline). Also worth a line in the description: Debug for GlueCatalog no longer prints the raw props, so the AWS secret key and session token stop showing up in debug output. (this could be a follow up, not a blocking things)
| }; | ||
| let kms_client = match self.kms_client_factory { | ||
| Some(factory) => Some(factory.create_kms_client(&self.config.props).await?), | ||
| Some(factory) => Some(factory.create_kms_client(&props).await?), |
There was a problem hiding this comment.
This now passes uri/catalog_id/warehouse to create_kms_client (and to FileIO via file_io_props), where the old code stripped them first. Fine since it's the intent, but nothing pins it. I'd add a small test with a stub KmsClientFactory that records the map it receives and returns an error, so load stops before GlueCatalog::new, then assert the recorded map equals the full input. That way nobody quietly brings the filter back later.
| /// Glue Catalog configuration | ||
| pub(crate) struct GlueCatalogConfig { | ||
| name: Option<String>, | ||
| #[derive(Debug, Properties)] |
There was a problem hiding this comment.
The rename dropped the old doc line, I'd keep a one-liner:
| #[derive(Debug, Properties)] | |
| /// Glue catalog properties parsed from the `load` property map. | |
| #[derive(Debug, Properties)] |
|
|
||
| #[tokio::test] | ||
| async fn test_load_rejects_missing_or_empty_warehouse() { | ||
| for props in [ |
There was a problem hiding this comment.
If this fails, the output won't say which of the two cases tripped. I'd label them (or split into two tests):
for (case, props) in [
("missing", HashMap::new()),
("empty", HashMap::from([(GLUE_CATALOG_PROP_WAREHOUSE.to_string(), String::new())])),
] {
// ...
assert_eq!(error.kind(), ErrorKind::DataInvalid, "{case}");
assert_eq!(error.message(), "Catalog warehouse is required", "{case}");
}
|
Hi, @laskoviymishka THanks for review, I created a pr for following up you mentioned: #3478 PTAL |
Record the full map received by the KMS factory and check initialization error propagation. Label missing and empty warehouse cases and document the crate-private Glue properties. Follow-up to apache#3102. Generated-by: Codex
3 participants
Which issue does this PR close?
What changes are included in this PR?
Use the shared
Propertiesderive for crate-private Glue catalog settings, following the Memory catalog pattern. Warehouse validation remains fail-fast, and the complete input property map is passed to AWS configuration, FileIO, and KMS.Are these changes tested?
Validated after merging
upstream/main, using the repository's pinnednightly-2026-04-16toolchain:cargo test --locked -p iceberg-catalog-glue --lib— 19 passed, including property parsing, defaults, and missing/empty warehouse validation.cargo clippy --locked -p iceberg-catalog-glue --all-targets -- -D warnings.cargo public-api -p iceberg-catalog-glue --all-features -ssmatches the committed public API.cargo fmt --all -- --checkandgit diff --check.AI Disclosure
Developed with assistance from OpenAI Codex. Review focus: forwarding modeled catalog properties alongside custom AWS, storage, and KMS properties, matching the Memory catalog's behavior.