kbendick commented on pull request #3034: URL: https://github.com/apache/iceberg/pull/3034#issuecomment-907878130
> Hi @kbendick , thanks. > Update. Hi @hehuiyuan, thanks for clarifying the reasoning for this. I'd have one suggestion. The `mergeHiveConf` method is getting a little large. Could you possibly create a method like `hiveConfFromEnvVariables` or something and then call that in `mergeHiveConf`? I know the current `mergeHiveConf` also isn't a strict merging, but now that it's getting more complicated, that would be ideal. So at least for this PR, if you could update your additions to be coming from their own function or functions, that would be helpful in working towards that goal. If you want to wait until there's feedback from the more Flink-focused committers though (like openinx and Jingsong Li), then feel free. Wouldn't want to have my style request change be for nothing 🙂. But if these environment variables are commonly provided as you mentioned, I think that makes sense. -- 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]
