Sure Udo. Dan is exploring some ideas now.
All: let's consider this round of RFC closed (since it officially closed
yesterday) and we'll re-open it with a new deadline when we have those
changes in hand.
On Thu, Mar 12, 2020 at 8:26 AM Udo Kohlmeyer <u...@apache.com> wrote:
> Hi there Jake,
>
> Another twist to the story, but with a working (if unpolished ;) )
> prototype.
>
> It covers all bases of:
>
> * Type safety
> * Extensibility
> * Simple API design
> * API clarity
>
> It takes the best of all approaches.
>
> I like it!!
>
> +1 to this implementation.
>
> -1 to Bill's approach.
>
> @Bill, could we amend the RFC to favor this approach?
>
> --Udo
>
> On 3/11/20 11:30 PM, Jacob Barrett wrote:
> > -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
> >
> >
>
