Re: RFC - Client side configuration for a SNI proxy

2020-03-10 Thread Dan Smith
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  wrote:

> Corrections ( :-P ), my apologies...
>
> Iterable 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  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 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 
> wrote:
> >
> >> Yes it’s redundant.
> >>
> >> > On Mar 9, 2020, at 5:08 PM, Bill Burcham 
> >> 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 
> >> wrote:
> >> >>
> >> >> +1 to Anthony and John. See similar comments in the RFC.
> >> >>
> >>  On Mar 9, 2020, at 12:08 PM, Anthony Baker 
> >> 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 
> >> >> 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
> >> u

Re: RFC - Client side configuration for a SNI proxy

2020-03-10 Thread Udo Kohlmeyer

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  wrote:


Corrections ( :-P ), my apologies...

Iterable 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  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 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 

wrote:

Yes it’s redundant.


On Mar 9, 2020, at 5:08 PM, Bill Burcham 

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 

wrote:

+1 to Anthony and John. See similar comments in the RFC.


On Mar 9, 2020, at 12:08 PM, Anthony Baker 

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 

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

Re: RFC - Client side configuration for a SNI proxy

2020-03-10 Thread John Blum
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 {

  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 getProxy();

}

-j


On Tue, Mar 10, 2020 at 10:38 AM Udo Kohlmeyer  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  wrote:
> >
> >> Corrections ( :-P ), my apologies...
> >>
> >> Iterable 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  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 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 
> >> wrote:
>  Yes it’s redundant.
> 
> > On Mar 9, 2020, at 5:08 PM, Bill Burcham 
>  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 
>  wrote:
> >> +1 to Anthony and John. See similar comments in the RFC.
> >>
>  On Mar 9, 2020, at 12:08 PM, Anthony Baker 
>  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
>

Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest

2020-03-10 Thread Robert Houghton
@dsmith
The file change for determining the merge base was due to changing the
Concourse resource. The file encodes the merge-base SHA, regardless of
depth of the git checkout.

On Tue, Mar 3, 2020 at 10:06 AM Dan Smith  wrote:

> What's the current timeout for StressNewTest? Maybe if we just up the
> threshold to 100 tests or so and up the timeout to match we can catch
> pretty much all PRs.
>
> I'm not sure why the job is flagging more tests than it should. It looks
> like at some point @rhoughon changed it to read the merge base from some
> file created by concourse as an optimization [1] - I suspect maybe that
> file is inaccurate?
>
> I originally wrote this job. It's definitely not a panacea, it will only
> catch a new flaky test if
>  - the test is really flaky (likely to fail more than 1/50 times)
>  - the change actually happened in the test file itself, and not the
> product or some other test file.
>
> [1]
>
> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
>
> -Dan
>
> On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols  wrote:
>
> > We don’t tend to look too closely at successful PR checks to see whether
> > they actually checked anything at all.
> >
> > One example I found is
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > <
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > >:
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.
> >
> > Here are 92 more examples (url’s omitted for brevity — use the example
> > above as a template and just replace the last 4 digits):
> > 26 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6243)
> > 26 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6249)
> > 26 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6402)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6262)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6430)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6439)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6449)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6454)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6458)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6459)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6224)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6441)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6448)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6452)
> > 29 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6102)
> > 29 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6177)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 5939)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 5940)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 5949)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6473)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 5953)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6187)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6470)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6471)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6474)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 6475)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (build 5958)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.  (

Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest

2020-03-10 Thread Dan Smith
On Tue, Mar 10, 2020 at 10:51 AM Robert Houghton 
wrote:

> @dsmith
> The file change for determining the merge base was due to changing the
> Concourse resource. The file encodes the merge-base SHA, regardless of
> depth of the git checkout.
>
> Makes sense. But maybe the file is inaccurate? Seems like the most likely
reason why this job is flagging too many tests. Is there any way to recover
that file from the job owen pointed out -
https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278?
Or maybe we just need to add more logging to this job to figure out why
it's flagging too many tests, including this file.

-Dan


Re: RFC - Client side configuration for a SNI proxy

2020-03-10 Thread Dan Smith
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  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 {
>
>   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 getProxy();
>
> }
>
> -j
>
>
> On Tue, Mar 10, 2020 at 10:38 AM Udo Kohlmeyer  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  wrote:
> > >
> > >> Corrections ( :-P ), my apologies...
> > >>
> > >> Iterable 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  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 proxyConfigurations = ...;
> > >>>
> > >>> StreamSupport.stream(proxyConfiguration.spilterator(), false)
> > >>>  .filter(config -> config.getType.isSecure())
> > >>>  .ifPresent(config -> // do something with secure proxy).
> > >>>
> > >>>
> >

Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest

2020-03-10 Thread Owen Nichols
To recap, the original question was: should we change StressNewTest to pass 
ONLY if it is able to successfully run all new/changed tests 50 times 
(eliminating the existing loophole exempting PRs that touch 25 or more files)?

So far a handful of people have expressed support, but many have 
questions/concerns:
- One concern was mis-counting of test files touched.  As of today this is now 
fixed.
- A big concern was that it might become more difficult to pass all required PR 
checks.
- Another concern was that the current timeout of 6 hours might be too high / 
too low.
- An alternative was suggested to keep the loophole but maybe increase the 
threshold required to get the free pass.
- If we’re going to raise the bar on any required PR check, maybe we should 
consider making it non-required.

Let’s extend the comment period until the end of next week (Friday Mar 20) in 
hopes of converging on a solution everybody is happy with (even if it isn’t 
what was originally proposed).


> On Mar 6, 2020, at 4:51 PM, Donal Evans  wrote:
> 
> With fairly apt timing, we've recently had a PR merged (
> https://github.com/apache/geode/pull/4745) that introduced a test that has
> resulted in fairly consistently flaky failures in the pipeline (JIRA
> ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843).
> The PR was quite large and touched/created a lot of tests, so StressNewTest
> never ran on it:
> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given how
> frequently the test is failing in the pipeline (11 failures on various
> IntegrationTest jobs over the past 6 commits), it seems very likely that
> had StressNewTest been allowed to run, this issue would have been caught at
> the PR stage and could have been remedied then, instead of resulting in a
> nearly constantly red pipeline.
> 
> I've already given my +1 to this proposal, but this situation has prompted
> me to make it a +2.
> 
> On Fri, Mar 6, 2020 at 1:46 PM Donal Evans  wrote:
> 
>> With fairly apt timing, we've recently had a PR merged (
>> https://github.com/apache/geode/pull/4745) that introduced a test that
>> has resulted in fairly consistently flaky failures in the pipeline (JIRA
>> ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843).
>> The PR was quite large and touched/created a lot of tests, so StressNewTest
>> never ran on it:
>> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given
>> how frequently the test is failing in the pipeline (11 failures on various
>> IntegrationTest jobs over the past 6 commits), it seems very likely that
>> had StressNewTest been allowed to run, this issue would have been caught at
>> the PR stage and could have been remedied then, instead of resulting in a
>> nearly constantly red pipeline.
>> 
>> I've already given my +1 to this proposal, but this situation has prompted
>> me to make it a *+1*.
>> 
>> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade 
>> wrote:
>> 
>>> The stress test is to identify the flaky-ness within the test; and
>>> assuming
>>> any changes to the test may have introduced the flaky-ness.
>>> It's about paying the cost upfront or later when the test is determined to
>>> be flaky.
>>> If 25+ tests have been changed in a PR, the cost of running stress test
>>> for
>>> those;  and gating the PR for so long.
>>> Knowing how much pain it causes to fix a flaky test after a certain/long
>>> duration of time; I am +1 for doing this change.
>>> 
>>> On Tue, Mar 3, 2020 at 10:06 AM Dan Smith  wrote:
>>> 
 What's the current timeout for StressNewTest? Maybe if we just up the
 threshold to 100 tests or so and up the timeout to match we can catch
 pretty much all PRs.
 
 I'm not sure why the job is flagging more tests than it should. It looks
 like at some point @rhoughon changed it to read the merge base from some
 file created by concourse as an optimization [1] - I suspect maybe that
 file is inaccurate?
 
 I originally wrote this job. It's definitely not a panacea, it will only
 catch a new flaky test if
 - the test is really flaky (likely to fail more than 1/50 times)
 - the change actually happened in the test file itself, and not the
 product or some other test file.
 
 [1]
 
 
>>> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
 
 -Dan
 
 On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols 
>>> wrote:
 
> We don’t tend to look too closely at successful PR checks to see
>>> whether
> they actually checked anything at all.
> 
> One example I found is
> 
 
>>> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> <
> 
 
>>> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
>> :
> 32 is too many changed tests to stress test. Allowing this jo

Re: RFC - Client side configuration for a SNI proxy

2020-03-10 Thread Dan Smith
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  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  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 {
>>
>>   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 getProxy();
>>
>> }
>>
>> -j
>>
>>
>> On Tue, Mar 10, 2020 at 10:38 AM Udo Kohlmeyer  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  wrote:
>> > >
>> > >> Corrections ( :-P ), my apologies...
>> > >>
>> > >> Iterable 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  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

Re: RFC - Client side configuration for a SNI proxy

2020-03-10 Thread Jacob Barrett
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  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  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 {
>> 
>>  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 getProxy();
>> 
>> }
>> 
>> -j
>> 
>> 
>>> On Tue, Mar 10, 2020 at 10:38 AM Udo Kohlmeyer  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  wrote:
> 
> Corrections ( :-P ), my apologies...
> 
> Iterable 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  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() meth

Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest

2020-03-10 Thread Dan Smith
I checked, and 1% of our last 500 commits touched more than 30 tests. Of
those 1%, over half touched more than 100 tests. So I'd guess somewhere
around .5% of prs will fall under the proposed changes.

My preference would be to just raise the threshold given that we already
raised the timeout. That'll catch 99% of prs without making test
refactoring and cleanups that touch many tests hard.

But, I'm ok with removing the limit if people really feel that a manual
override process is better for that 1%.

-Dan
On Tue, Mar 10, 2020, 4:33 PM Owen Nichols  wrote:

> To recap, the original question was: should we change StressNewTest to
> pass ONLY if it is able to successfully run all new/changed tests 50 times
> (eliminating the existing loophole exempting PRs that touch 25 or more
> files)?
>
> So far a handful of people have expressed support, but many have
> questions/concerns:
> - One concern was mis-counting of test files touched.  As of today this is
> now fixed.
> - A big concern was that it might become more difficult to pass all
> required PR checks.
> - Another concern was that the current timeout of 6 hours might be too
> high / too low.
> - An alternative was suggested to keep the loophole but maybe increase the
> threshold required to get the free pass.
> - If we’re going to raise the bar on any required PR check, maybe we
> should consider making it non-required.
>
> Let’s extend the comment period until the end of next week (Friday Mar 20)
> in hopes of converging on a solution everybody is happy with (even if it
> isn’t what was originally proposed).
>
>
> > On Mar 6, 2020, at 4:51 PM, Donal Evans  wrote:
> >
> > With fairly apt timing, we've recently had a PR merged (
> > https://github.com/apache/geode/pull/4745) that introduced a test that
> has
> > resulted in fairly consistently flaky failures in the pipeline (JIRA
> > ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> ).
> > The PR was quite large and touched/created a lot of tests, so
> StressNewTest
> > never ran on it:
> > https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given
> how
> > frequently the test is failing in the pipeline (11 failures on various
> > IntegrationTest jobs over the past 6 commits), it seems very likely that
> > had StressNewTest been allowed to run, this issue would have been caught
> at
> > the PR stage and could have been remedied then, instead of resulting in a
> > nearly constantly red pipeline.
> >
> > I've already given my +1 to this proposal, but this situation has
> prompted
> > me to make it a +2.
> >
> > On Fri, Mar 6, 2020 at 1:46 PM Donal Evans  wrote:
> >
> >> With fairly apt timing, we've recently had a PR merged (
> >> https://github.com/apache/geode/pull/4745) that introduced a test that
> >> has resulted in fairly consistently flaky failures in the pipeline (JIRA
> >> ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> ).
> >> The PR was quite large and touched/created a lot of tests, so
> StressNewTest
> >> never ran on it:
> >> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given
> >> how frequently the test is failing in the pipeline (11 failures on
> various
> >> IntegrationTest jobs over the past 6 commits), it seems very likely that
> >> had StressNewTest been allowed to run, this issue would have been
> caught at
> >> the PR stage and could have been remedied then, instead of resulting in
> a
> >> nearly constantly red pipeline.
> >>
> >> I've already given my +1 to this proposal, but this situation has
> prompted
> >> me to make it a *+1*.
> >>
> >> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade 
> >> wrote:
> >>
> >>> The stress test is to identify the flaky-ness within the test; and
> >>> assuming
> >>> any changes to the test may have introduced the flaky-ness.
> >>> It's about paying the cost upfront or later when the test is
> determined to
> >>> be flaky.
> >>> If 25+ tests have been changed in a PR, the cost of running stress test
> >>> for
> >>> those;  and gating the PR for so long.
> >>> Knowing how much pain it causes to fix a flaky test after a
> certain/long
> >>> duration of time; I am +1 for doing this change.
> >>>
> >>> On Tue, Mar 3, 2020 at 10:06 AM Dan Smith  wrote:
> >>>
>  What's the current timeout for StressNewTest? Maybe if we just up the
>  threshold to 100 tests or so and up the timeout to match we can catch
>  pretty much all PRs.
> 
>  I'm not sure why the job is flagging more tests than it should. It
> looks
>  like at some point @rhoughon changed it to read the merge base from
> some
>  file created by concourse as an optimization [1] - I suspect maybe
> that
>  file is inaccurate?
> 
>  I originally wrote this job. It's definitely not a panacea, it will
> only
>  catch a new flaky test if
>  - the test is really flaky (likely to fail more than 1/50 times)
>  - the change actually happened in the test file itself, and not the
>