[#4364] feat(iceberg): Support GCS storage for Iceberg REST server #4627
Conversation
|
@jerryshao @yuqi1129 @diqiu50 please help to review when you are free, thanks |
| @@ -161,6 +161,7 @@ iceberg-aws = { group = "org.apache.iceberg", name = "iceberg-aws", version.ref | |||
| iceberg-core = { group = "org.apache.iceberg", name = "iceberg-core", version.ref = "iceberg" } | |||
| iceberg-api = { group = "org.apache.iceberg", name = "iceberg-api", version.ref = "iceberg" } | |||
| iceberg-hive-metastore = { group = "org.apache.iceberg", name = "iceberg-hive-metastore", version.ref = "iceberg" } | |||
| iceberg-gcp = { group = "org.apache.iceberg", name = "iceberg-gcp", version.ref = "iceberg" } | |||
There was a problem hiding this comment.
Should this be added. if uses want to use, it's add the jars to class path himself
There was a problem hiding this comment.
gcp is a general storage and Iceberg gcp package jar is light without the GCP dependences, so I prefer to keep it in Gravitino.
|
�I'm OK with the change here. But it is not a good design to support adding cloud storage one by one continuously. We should figure out a framework to support adding by users. |
|
|
||
| For other Iceberg GCS properties not managed by Gravitino like `gcs.project-id`, you could config it directly by `gravitino.bypass.gcs.project-id`. | ||
|
|
||
| Please make sure the credential file is accessible by Gravitino, like using `export GOOGLE_APPLICATION_CREDENTIALS=/xx/application_default_credentials.json` before Gravitino server is started. |
There was a problem hiding this comment.
Does GCS need configurations like access key ?
There was a problem hiding this comment.
GCS access key and secrect key are encoded in application_default_credentials.json
For more general storage, we could define properties explicitly, for others use #4896 to support. |
What changes were proposed in this pull request?
Support GCS storage for Iceberg REST server
Why are the changes needed?
Fix: #4364
Does this PR introduce any user-facing change?
No
How was this patch tested?