Bill,
thank you for writing up *_our_* discussion of wrt this matter.
+1 on the new API suggestion
+1 to the future expansion, using an SPI approach to extend into support
SOCKS5 aswell.
Great team effort to get this one over the line.
--Udo
On 3/11/20 3:03 PM, Bill Burcham wrote:
In order to expeditiously address the current need (SNI proxy
configuration) and leave the door open to possible future expansion to
other proxy protocols, possibly via an SPI as Jacob alluded, I'd like to
propose modifying the current RFC as follows:
* eliminate the ProxyType enum
* change the name of the setProxy methods to stipulate that it is the
_address_ that is being set e.g. setProxyAddress, setPoolProxyAddress
* separate the proxyAddress into two parts: host and port. That gives us a
little stronger typing in line with Anthony's concerns and Dan's latest
proposal
So I'm proposing this:
PoolFactory {
setProxyAddress(String host, int port);
}
ClientCacheFactory {
setPoolProxyAddress(String host, int port);
}
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.
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.
Can we all live with this approach? I'm looking for your +1s please!
On Tue, Mar 10, 2020 at 4:54 PM Jacob Barrett <jbarr...@pivotal.io> wrote:
I think we should look at it from the standpoint of an SPI. The proxy work
should be pluggable. One implementation we will provide out of the box is
SNI.
With this in mind geode-core (or better yet geode-proxy-spi) should only
define a few interfaces. ProxyConfiguration and ProxyFactory.
The code that creates client socket in geode-core should depend on the
geode-proxy module that has logic to service discover ProxyFactory. Given a
ProxyConfiguration instance it should be able to load/discover/interrogate
for the correct ProxyFactory. The socket factory asks the ProxyFactory for
a proxied socket.
The geode-proxy-sni module would then implement all Thai for SNI proxy.
Pluggable and modular.
-Jake
On 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
