[#4551] feat(iceberg): add S3 and GCS support for IcebergRESTService docker image #5243
Conversation
FANNG1
changed the title
|
@jerryshao @yuqi1129 @jerqi please help to review, thanks |
|
|
||
| cd ${iceberg_rest_server_dir} | ||
|
|
||
| python bin/rewrite_config.py |
There was a problem hiding this comment.
Is the python command installed by default?
There was a problem hiding this comment.
yes, it's installed in docker image
dev/docker/iceberg-rest-server/iceberg-rest-server-dependency.sh
Outdated
Show resolved
Hide resolved
|
Please update the doc |
updated, please help to review again |
| config_map = parse_config_file(config_file_path) | ||
|
|
||
| for k, v in init_config.items(): | ||
| update_config(config_map, k, v) |
There was a problem hiding this comment.
You can make the dict config_map as the method parameter and do not need to call the method several time.
There was a problem hiding this comment.
it could improve the performance, but the current implementation is more extensiable, I think this is more important, WDYT?
There was a problem hiding this comment.
What do you mean by more extensiable? do you mean we can easily support more properties in this way?
There was a problem hiding this comment.
yes, like what env_map is processed
There was a problem hiding this comment.
I mean we can change it into the following kind:
update_config(config_map, init_config):
for key, value in init_config.items():
config_map[config_prefix + key] = valueThere was a problem hiding this comment.
Then how to process env_map?
There was a problem hiding this comment.
Since the method contains only one simple sentence, I can't find the difference that you replace it with config[config_prefix + key] = value directly if insist on this the way.
def update_config(config, key, value):
config[config_prefix + key] = value
There was a problem hiding this comment.
If just for performance, I prefer not to change it. You suggest to pass map to update_config, seems difficult to process env_map ?
|
@yuqi1129 any other comments? @jerryshao do you have time to review again? |
I have no further comments. |
…docker image (#5243) ### What changes were proposed in this pull request? 1. add AWS and GCP bundle jar to IcebergRESTServer docker image 2. use environment variable to change the config ### Why are the changes needed? Fix: #4551 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? run SQL with access S3 and GCS data
What changes were proposed in this pull request?
Why are the changes needed?
Fix: #4551
Does this PR introduce any user-facing change?
no
How was this patch tested?
run SQL with access S3 and GCS data