Re: RFC - Client side configuration for a SNI proxy
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
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
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
@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
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
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
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
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
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
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 >
