CTTY commented on code in PR #3090:
URL: https://github.com/apache/iceberg-rust/pull/3090#discussion_r3883314556
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -59,13 +60,14 @@ use crate::Result;
/// .with_prop("key", "value")
/// .build();
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct FileIO {
/// Storage configuration containing properties
config: StorageConfig,
Review Comment:
this is a good point, we are basically thinking if "credentials" should be
serialized and passed across machines.
for now I don't have a better idea to solve this other than filtering the
properties using some keywords like `*secret*, *token*, *key*, *password*`.
That is a bad idea because users would find it very hard to find out what
configs have been filtered during serde, and what configs they need to
regenerate on the new machine.
Also not redacting props in `StorageConfig` is not a new behavior so I'm
leaning toward keeping this as it is for now
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -59,13 +60,14 @@ use crate::Result;
/// .with_prop("key", "value")
/// .build();
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct FileIO {
/// Storage configuration containing properties
config: StorageConfig,
/// Factory for creating storage instances
factory: Arc<dyn StorageFactory>,
Review Comment:
+1 we should document it
--
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]
