After some side discussions, I think different SniProxyConfiguration,
SocksProxyConfiguration, etc. interfaces are more than we need. Just having
a type, host and port should support any proxies we might see in the
foreseeable future.
I like this better: Option A
setProxy(ProxyType type, String host, int port)
But I could live with a ProxyConfiguration interface that just provides
those three fields, Option B
setProxy(ProxyConfiguration)
interface ProxyConfiguration {
ProxyType type;
String host;
int port;
}
On Tue, Mar 10, 2020 at 11:19 AM Dan Smith <dsm...@pivotal.io> wrote:
> 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
>>
>
