apache / iceberg-rust

Apache Iceberg
https://rust.iceberg.apache.org/
Apache License 2.0
483 stars 99 forks source link

idea(catalog/rest): Separate static and dynamic props #430

Closed Xuanwo closed 4 days ago

Xuanwo commented 5 days ago

Current reset catalog's init logic doesn't look good:

pub async fn new(config: RestCatalogConfig) -> Result<Self> {
    let mut catalog = Self {
        client: config.try_create_rest_client()?,
        config,
    };
    catalog.fetch_access_token().await?;
    catalog.client = catalog.config.try_create_rest_client()?;
    catalog.update_config().await?;
    catalog.client = catalog.config.try_create_rest_client()?;

    Ok(catalog)
}

We need to:

I suggest separating static and dynamic properties. Keep the configuration and HTTP client static, unchanged. Store dynamic properties separately and merge them only when sending a request. This approach could also help resolve issue #422.

liurenjie1024 commented 5 days ago

The problem is that the first update_config may return things that may change behavior of rest client, for example headers, or relocated url, IIRC.

Xuanwo commented 5 days ago

Got it. I have a better idea, let's discuss on the PR.

TennyZhuang commented 5 days ago

BTW should we use a strong-typed Config/Props instead of a Hashmap? Do we have discussions before?

Xuanwo commented 5 days ago

BTW should we use a strong-typed Config/Props instead of a Hashmap? Do we have discussions before?

I like this idea. Please feel free to start a discussion about it. Actually, I'm not sure how many valid options are available in the props.

liurenjie1024 commented 4 days ago

BTW should we use a strong-typed Config/Props instead of a Hashmap? Do we have discussions before?

It seems that there is no clear definition of all props.