The other reason I like setProxy(SniProxyConfiguration) is that it makes it
clear to the user what a legal value to pass in is. with
setProxy(ProxyConfiguration) the user can be mislead into thinking they are
supposed to implement ProxyConfiguration, but at runtime our code will
break if the object is not a SniProxyConfiguration. Anytime we can take a
runtime check for a bad argument and make it an API constraint that is
enforced at compile time I think that's a win.
That said, I don't really care that much. I personally think even
setSniProxy(String host, int port) or setProxy(ProxyType.SNI, String host,
int port) would be fine - we may someday add SOCKS5 support, but I doubt
we'll ever hardcode support for more than 2 or 3 types of proxies. We'd be
more likely to provide a more generic way to completely override the
connection code.
-Dan
On Tue, Mar 10, 2020 at 10:44 AM John Blum <jb...@pivotal.io> wrote:
> Again, we should keep the API footprint *small* and quit adding overrides
> for every type of Proxy we would (potentially) support. What is the point
> of an abstraction/type-hierarchy if you don't use it?
>
> The Enum would give users the possible range of currently supported Proxies
> anyway.
>
> Additionally, if you wanted something more specific, then you could
> introduce a generic type signature, which is more useful for return types,
> but work equally well for parameters as well...
>
> interface PoolFactory<P extends ProxyConfiguration> {
>
> void setProxy(P config);
>
> }
>
> Of course, this assumes you would have multiple different types of
> PoolFactory, which Geode does not currently, but could.
>
> Generic parameters are more useful for return types, and the entire
> PoolFactory, and don't necessarily need a generic type signature on the
> enclosing type, for example:
>
> interface PoolFactory {
>
> <P extends PoolConfiguration> P getProxy();
>
> }
>
> -j
>
>
> On Tue, Mar 10, 2020 at 10:38 AM Udo Kohlmeyer <u...@apache.com> wrote:
>
> > I disagree.
> >
> > I think /setProxy(ProxyConfiguration)/ is 1st prize.
> >
> > If we are concerned that users will not know WHAT options are
> > available.. We could always have a static builder for our supported
> > options.
> >
> > --Udo
> >
> > On 3/10/20 10:07 AM, Dan Smith wrote:
> > > Ok, how about this?
> > >
> > > setProxy(SniProxyConfiguration config)
> > >
> > > interface SniProxyConfiguration extends ProxyConfiguration {
> > > static SniProxyConfiguration create(String host, int port);
> > > String getHost();
> > > int getPort()
> > > }
> > >
> > > The main difference here from John's proposal being that setProxy
> takes a
> > > SniProxyConfiguration, rather than the base ProxyConfiguration. We can
> > > overload setProxy later if needed.
> > >
> > > The reason I'm not as keen on setProxy(ProxyConfiguration) is that it
> is
> > > hard for the user to discover the different types of ProxyConfiguration
> > > subclasses and know what is supported.
> > >
> > > -Dan
> > >
> > > On Mon, Mar 9, 2020 at 11:23 PM John Blum <jb...@pivotal.io> wrote:
> > >
> > >> Corrections ( :-P ), my apologies...
> > >>
> > >> Iterable<ProxyConfiguration> proxyConfigurations = ...;
> > >>
> > >> StreamSupport.stream(proxyConfiguration*s*.*spliterator*(), false)
> > >> .filter(config -> config.getType.isSecure()) *// This could be
> > >> improved; see below...*
> > >> .*forEach*(config -> // do something with *each* secure proxy
> > >> *config*).
> > >>
> > >> Although, I am sure you got the point, ;-)
> > >>
> > >> With improvement:
> > >>
> > >> interface ProxyConfiguration {
> > >>
> > >> ProxyType getType();
> > >>
> > >> // null-safe!
> > >> default boolean isSecure() {
> > >> ProxyType type = getType();
> > >> return type != null && type.isSecure();
> > >> }
> > >> }
> > >>
> > >> Then:
> > >>
> > >> StreamSupport.stream(..)
> > >> * .filter(ProxyConfiguration::isSecure)*
> > >> .forEach(...);
> > >>
> > >>
> > >> Again, completely contrived.
> > >>
> > >> Cheers!
> > >> -j
> > >>
> > >>
> > >>
> > >> On Mon, Mar 9, 2020 at 11:14 PM John Blum <jb...@pivotal.io> wrote:
> > >>
> > >>> Yes, it's redundant (i.e. Enum + class type).
> > >>>
> > >>> However, having an Enum in addition to a specific type (1 reason I
> > >>> defaulted the getType() method) can still be useful, such as in a
> > switch
> > >>> statement for example. Enums are, well, easier to enumerate (useful
> in
> > >>> Streams with filters). Maybe you are going to classify certain
> Proxy's
> > >>> by Enum type, for example:
> > >>>
> > >>> enum ProxyType {
> > >>>
> > >>> ONE,
> > >>> TWO,
> > >>> THREE;
> > >>>
> > >>> // Contrived example
> > >>> public boolean isSecure() {
> > >>> return Arrays.asList(ONE, THREE).contains(this);
> > >>> }
> > >>> }
> > >>>
> > >>> Then:
> > >>>
> > >>> Iterable<ProxyConfiguration> proxyConfigurations = ...;
> > >>>
> > >>> StreamSupport.stream(proxyConfiguration.spilterator(), false)
> > >>> .filter(config -> config.getType.isSecure())
> > >>> .ifPresent(config -> // do something with secure proxy).
> > >>>
> > >>>
> > >>> Food for thought.
> > >>>
> > >>> -j
> > >>>
> > >>>
> > >>>
> > >>> On Mon, Mar 9, 2020 at 7:11 PM Jacob Barrett <jbarr...@pivotal.io>
> > >> wrote:
> > >>>> Yes it’s redundant.
> > >>>>
> > >>>>> On Mar 9, 2020, at 5:08 PM, Bill Burcham <bill.burc...@gmail.com>
> > >>>> wrote:
> > >>>>> What I like about John's full-fledged-class-per-proxy-kind is that
> > >>>>> everything that can potentially vary with proxy kind is all
> together
> > >> in
> > >>>> a
> > >>>>> single object.
> > >>>>>
> > >>>>> That being said, John, in your SniProxyConfiguration, it seems to
> me
> > >>>> that
> > >>>>> the class itself (SniProxyConfiguration) could easily stand for the
> > >>>> proxy
> > >>>>> "type". If that's true then we could view ProxyType as redundant
> and
> > >>>> simply
> > >>>>> eliminate it right?
> > >>>>>
> > >>>>>> On Mon, Mar 9, 2020 at 2:41 PM Jacob Barrett <jbarr...@pivotal.io
> >
> > >>>> wrote:
> > >>>>>> +1 to Anthony and John. See similar comments in the RFC.
> > >>>>>>
> > >>>>>>>> On Mar 9, 2020, at 12:08 PM, Anthony Baker <aba...@pivotal.io>
> > >>>> wrote:
> > >>>>>>> I’m not suggesting encoding the the proxy type in the URI. Just
> > >>>>>> wondering if we can support stronger typing than String for
> defining
> > >>>>>> host/port/url configuration. As John notes, later in the thread,
> > >>>> perhaps
> > >>>>>> using a configuration interface may help.
> > >>>>>>> Anthony
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>> On Mar 9, 2020, at 11:11 AM, Bill Burcham <
> bill.burc...@gmail.com
> > >
> > >>>>>> wrote:
> > >>>>>>>> Anthony and Jacob, I can see how the proposed ProxyType
> parameter
> > >>>> could
> > >>>>>> fit
> > >>>>>>>> into the scheme part of a a URI. However, the problem that
> > >>>> introduces is
> > >>>>>>>> that we would then have to pick (named) URL schemes to support.
> > But
> > >>>> URL
> > >>>>>>>> schemes are standardized and it's not obvious which of the
> > standard
> > >>>> ones
> > >>>>>>>> might apply here.
> > >>>>>>>>
> > >>>>>>>> If we couldn't adopt a standard scheme, we'd have to make one
> up.
> > >> At
> > >>>>>> that
> > >>>>>>>> point I question the value of putting the (made-up) scheme into
> a
> > >> URI
> > >>>>>>>> string.
> > >>>>>>>>
> > >>>>>>>> For this reason, I am a fan of the ProxyType parameter over a
> > >> made-up
> > >>>>>> URL
> > >>>>>>>> scheme.
> > >>>>>>>>
> > >>>>>>>> That leaves open Anthony's other idea: eliminating the ProxyType
> > >>>>>> parameter
> > >>>>>>>> in favor of a separate method to set each kind of proxy. In the
> > >>>> current
> > >>>>>>>> RFC, that's just one, e.g. setPoolProxyWithSNI. I guess that
> comes
> > >>>> down
> > >>>>>> to:
> > >>>>>>>> what's the likelihood of us supporting other proxy types soon,
> and
> > >>>> then
> > >>>>>>>> what's the value of having a single method (and multiple enums)
> > >>>> versus
> > >>>>>>>> multiple methods (and no enum). If we thought the proxyAddress
> > >>>> parameter
> > >>>>>>>> would carry different information across proxy types that might
> > >> tilt
> > >>>> us
> > >>>>>>>> toward the separate methods. The two on the table, however,
> (SNI,
> > >>>>>> SOCKS5)
> > >>>>>>>> both have identical proxyAddress information.
> > >>>>>>>>
> > >>>>>>>> On Mon, Mar 9, 2020 at 10:54 AM Bill Burcham <
> > >> bill.burc...@gmail.com
> > >>>>>> wrote:
> > >>>>>>>>> By popular demand we are extending the RFC review period. I
> know
> > >> Udo
> > >>>>>> asked
> > >>>>>>>>> for Friday (and Joris +1'd it), but since this is a small RFC,
> > >> we'd
> > >>>>>> like to
> > >>>>>>>>> try to close it by Wednesday, March 11, ok?
> > >>>>>>>>>
> > >>>>>>>>> On Mon, Mar 9, 2020 at 10:39 AM Jacob Barrett <
> > >> jbarr...@pivotal.io>
> > >>>>>> wrote:
> > >>>>>>>>>> I raised similar concerns as a comment in the RFC.
> > >>>>>>>>>>
> > >>>>>>>>>>> On Mar 9, 2020, at 10:29 AM, Anthony Baker <
> aba...@pivotal.io>
> > >>>>>> wrote:
> > >>>>>>>>>>> Given this new API:
> > >>>>>>>>>>>
> > >>>>>>>>>>> setPoolProxy(ProxyType type, String proxyAddress)
> > >>>>>>>>>>>
> > >>>>>>>>>>> The ProxyType enum seems to be a look ahead at supporting
> other
> > >>>> kinds
> > >>>>>>>>>> of proxies. What is your thinking about using the enum vs
> > >> specific
> > >>>>>> named
> > >>>>>>>>>> API’s (e.g. setPoolProxyWithSNI).
> > >>>>>>>>>>> Currently the definition of the proxyAddress seems to be
> > >>>> dependent of
> > >>>>>>>>>> the proxy type. Did you consider stronger typing using an URI
> > >>>>>> parameter
> > >>>>>>>>>> type?
> > >>>>>>>>>>> Anthony
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>> On Mar 6, 2020, at 11:04 AM, Bill Burcham <
> > >>>> bill.burc...@gmail.com>
> > >>>>>>>>>> wrote:
> > >>>>>>>>>>>> Please review the RFC for *Client side configuration for a
> SNI
> > >>>>>> proxy*:
> > >>>>>>>>>>>>
> > >>
> >
> https://cwiki.apache.org/confluence/display/GEODE/Client+side+configuration+for+a+SNI+proxy
> > >>>>>>>>>>>> Please comment by Monday, March 9, 2020.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Regards,
> > >>>>>>>>>>>> Bill and Ernie
> > >>>>>>
> > >>>
> > >>> --
> > >>> -John
> > >>> Spring Data Team
> > >>>
> > >>
> > >> --
> > >> -John
> > >> Spring Data Team
> > >>
> >
>
>
> --
> -John
> Spring Data Team
>
