-1
I hate to do this but I really feel like we went backwards on this change.
> On Mar 11, 2020, at 3:03 PM, Bill Burcham <bill.burc...@gmail.com> wrote:
> PoolFactory {
> setProxyAddress(String host, int port);
> }
>
> ClientCacheFactory {
> setPoolProxyAddress(String host, int port);
> }
It gives the user no information about what type of proxy is in use. So
documentation must specify that it is only SNI. It assumes that SNI and any
other proxy would have a host and port only.
Also consider if we used SRV records to discover the proxy, would I need to set
hostname to null or the service name, what about port since it comes from the
SRV record. Would I set port to 0?.
What about default ports for well know proxy services like SOCKS? Again, 0?
> This should address Johns desire for an API that doesn't grow with each new
> proxy type. And it should address Udo's desire for extensibility.
I really struggle to see how this satisfies future extensibility given the
parameter lock in. Providing a class allows the proxy to define all its
options. Consider SniProxyConfig that takes no parameters, it could use the SRV
records for “_sniproxy._tcp” queried on the current domain to discover the
proxy. Or if given just a hostname could SRV query “_sniproxy._tcp.hostname”
and fall back to A “hostname” on the default port.
>
> This API sets the stage for a potential follow-on RFC to support other
> proxy types. One way that might work would be addition of
> setProxyType(String type)/setPoolProxyType(String type) methods. That RFC
> might reserve the "SNI" proxy type. That RFC might specify an SPI (per
> Jacob's proposal) that would let Geode users register their own proxy types
> e.g. SOCKS.
Adding disconnected method to set the proxy type by string doesn’t sit well for
me. What strings are valid? When we add this method what does it mean if we
don’t set it, its always SNI?
How would you deprecate a proxy? If the proxy configuration is a class I can
discover it in my IDE and deprecate the class when not supported anymore. Even
the original Enum idea could have deprecated enum values.
I would vote for the original RFC over this given that it weakens the type
safety of the API.
To illustrate my point on the SPI see
https://github.com/apache/geode/compare/develop...pivotal-jbarrett:wip/proxy-spi
<https://github.com/apache/geode/compare/develop...pivotal-jbarrett:wip/proxy-spi>.
Its a quick hack and some things might not be perfect, like discovering the
factories, but it should get the idea across.
-Jake
