Core, Rest: Enable useSystemProperties on RESTClient

[munendrasn]

• When useSystemProperties on the httpClient, it reads the proxy settings if not configured explicitly from system properties.
• Introduce new rest client property enable or disable consuming systemProperty

cc @danielcweeks @amogh-jahagirdar @nastra

[munendrasn]

@amogh-jahagirdar @nastra could you please take look?

[nastra]

@munendrasn can you please add some details why this functionality is needed? Having more and more configuration flags just makes it quite complicated for users

[munendrasn]

@nastra

Agreed on having more configuration flags.. The original intent was to enable useSystemProperties on HttpClient too as mentioned in the Description. This would help in consuming the Java system properties directly for cases like Http Proxy, or keepLive configuration

Reasoning for introducing new configuration property to enable or disable usingSystemProperties :
One sample usecase, where we need to connect to Different RESTCatalog impl
One of the impl requires mTLS connection and other doesn't. We specify trustStore and keyStore impl via Java System properties
We would like to consume these systemProperties in one case but not the other case.. hence, configuration property

[nastra]

We would like to consume these systemProperties in one case but not the other case.. hence, configuration property

What about not setting these system properties in the second use case?

[munendrasn]

What about not setting these system properties in the second use case?

We access both catalogs in same process/job.. a case of custom catalog federation

[munendrasn]

Should I consider splitting the changes into different PR.. ie, enabling the systemProperties on the client and making usage of system properties configurable through property

[munendrasn]

@nastra @danielcweeks could you please review this?

[nastra]

I'm really hesitant to introduce a flag to control whether things get read from system properties or not. Let's wait for feedback from @danielcweeks

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

[gaborkaszab]

I saw some hesitance in the comments whether to have this change or not. I don't have a strong opinion myself, but for me it seems that since the HttpClientBuilder already has this option we just make it possible to use it for Iceberg's HTTP clients, so would make sense in a way. So if there are users wanting to use it, they would have it.
What I found is that with the current proposal we seem to change the default to use system props. I don't think we should do that, let's keep the defaults as they are now.

[gaborkaszab]

If I'm not mistaken the current behaviour doesn't use system properties. With this change we seem to change that behaviour and one would have to turn that off manually by setting a property to false.

[munendrasn]

systemProperties are used for ConnectionManager but not on Client. Not sure, what would the ideal behaviour in this case.

[munendrasn]

@nastra Created another PR with possible alternative solution #11979