xanderbailey commented on PR #2383: URL: https://github.com/apache/iceberg-rust/pull/2383#issuecomment-4419229901
Tried out using interior mutability for the keys here [4c1a33c](https://github.com/apache/iceberg-rust/pull/2383/commits/4c1a33cdd491330bb8878d4fc140efd3f4d9782f) This change would make `commit` look something like this: first ask to encrypted the key_metadata and then get all of the encryption keys, diff them against the existing keys. ``` rust let mut table_updates = Vec::new(); let encryption_key_id = if let Some(ml_km) = ml_key_metadata { let em = self.table.encryption_manager().ok_or_else(|| { Error::new( ErrorKind::FeatureUnsupported, "Encrypted manifest list but no EncryptionManager configured on table", ) })?; // Wrap the DEK; manager stores the wrapped entry (and any new KEK) internally. let entry_key_id = em.encrypt_manifest_list_key_metadata(&ml_km).await?; // Diff: emit AddEncryptionKey for everything the manager has that // isn't already in TableMetadata.encryption_keys. let existing_ids: HashSet<&str> = self .table .metadata() .encryption_keys_map() .keys() .map(String::as_str) .collect(); for (id, key) in em.encryption_keys() { if !existing_ids.contains(id.as_str()) { table_updates.push(TableUpdate::AddEncryptionKey { encryption_key: key, }); } } Some(entry_key_id) } else { None }; ``` I personally don't love the interior mutability here, the side-effect nature of `encrypt_manifest_list_key_metadata` now means the calling code must first call `encrypt_manifest_list_key_metadata` before getting all keys which seems a little subtle to me. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
