Re: [DISCUSS] Time to cut Geode 1.10.0?

[Jared Stewart]

What was missing from the RFC process for the cluster management service?
I saw a [Discuss] thread for it, as well as a proposal at
https://cwiki.apache.org/confluence/display/GEODE/Cluster+Management+Service

On Tue, Jul 23, 2019 at 10:02 AM Udo Kohlmeyer  wrote:

> I don't believe we should be including anything into the Geode release
> that has not gone through the correct process of feature proposal.
>
> All work under the experimental cluster management service has not yet
> been approved by the agreed upon RFC process.
>
> I don't believe we should be including this work, experimental or
> otherwise.
>
> --Udo
>
> On 7/22/19 4:51 PM, Alexander Murmann wrote:
> > Udo, do you mind explaining how the RFC process comes into this? Are you
> > suggesting that we should wait if an RFC had a target release attached?
> >
> > On Mon, Jul 22, 2019 at 4:47 PM Udo Kohlmeyer  wrote:
> >
> >> I don't think we need to wait for this, as there has been no RFC process
> >> followed.
> >>
> >> --Udo
> >>
> >> On 7/22/19 3:38 PM, Jinmei Liao wrote:
> >>> Work is still being planned to move the cluster management rest service
> >>> under an experimental version flag and use a geode property to turn it
> >>> on/off. I would say we are ready to cut the geode 1.10.0 after that
> work
> >> is
> >>> complete.
> >>>
> >>> On Mon, Jul 22, 2019 at 3:24 PM Alexander Murmann  >
> >>> wrote:
> >>>
>  Hi everyone!
> 
>  We released Geode 1.9.0 on April 25th. That's about 3 months ago. End
> of
>  last year we discussed releasing quarterly. In the past we've had
> about
> >> a
>  month between cutting a release branch and actually shipping our new
> >> minor.
>  This means we are already behind our target release cadence.
> 
>  What are your thoughts on cutting a 1.10.0 release branch this week?
> 
>  Would anyone like to volunteer to be the release manager for geode
> >> 1.10.0?
>  Thank you all!
> 
>

Re: Is the documentation wrong here?

[Jared Stewart]

Yes, I believe the docs there are out of date and need to be updated.  This
change in the naming of deployed jars was introduced in Geode 1.2 by
https://github.com/apache/geode/pull/429.

On Tue, Jun 4, 2019 at 7:27 AM liyuj <18624049...@163.com> wrote:

> Hi,
>
>
> https://geode.apache.org/docs/guide/19/configuring/cluster_config/deploying_application_jars.html
>
> This document has such a paragraph:
>
>
> Versioning of JAR Files
>
> When you deploy JAR files to a cluster or member group, the JAR file is
> modified to indicate version information in its name. Each JAR filename
> is prefixed with|vf.gf#|and  contains a version
> number at the end of the
> filename. For example, if you deploy|MyClasses.jar|five times, the
> filename is displayed as|vf.gf#MyClasses.jar#5|when
>  you list all
> deployed jars.
>
> but,in my environment, it is shown as follows:
>
> gfsh>list deployed
> Member  |  JAR   | JAR Location
> --- | -- |
> ---
> server1 | ra.jar | /media/liyujue/data/geode/server1/ra.v1.jar
> server1 | mx4j-3.0.2.jar |
> /media/liyujue/data/geode/server1/mx4j-3.0.2.v1.jar
> server2 | ra.jar | /media/liyujue/data/geode/server2/ra.v1.jar
> server2 | mx4j-3.0.2.jar |
> /media/liyujue/data/geode/server2/mx4j-3.0.2.v1.jar
>
> Is the documentation wrong here?
>
>

Re: Backwards compatibility issue with JSONFormatter

[Jared Stewart]

It looks like the japi-compliance-checker tool to which Anthony linked is
available as a gradle plugin: https://github.com/melix/japicmp-gradle-plugin

On Tue, May 14, 2019 at 5:07 PM Ryan McMahon  wrote:

> Fixed in
>
> https://github.com/apache/geode/commit/cc0b37a504480f554b1884491f44a3cca4113ef5
>
> On Tue, May 14, 2019 at 2:06 PM Ryan McMahon  wrote:
>
> > Thanks Anthony!  Have we considered using a compliance checker in our CI
> > like the one in the first link?
> >
> > Side note - I think that varargs is an interesting case that I don't see
> > called out in those links.  Since varargs is just syntactic sugar, it is
> > ultimately the varargs parameter is converted to an array in the
> bytecode.
> > So in the JSON formatter change:
> >
> > fromJSON(byte[] jsonByteArray, String... identityFields)
> >
> >
> > becomes
> >
> > fromJSON(byte[] jsonByteArray, String[] identityFields)
> >
> >
> > Hence the breakage of the ABI.
> >
> > Ryan
> >
> > On Tue, May 14, 2019 at 1:33 PM Anthony Baker  wrote:
> >
> >> Here are a few links on API compatibility:
> >>
> >> https://lvc.github.io/japi-compliance-checker/#Examples
> >> https://wiki.eclipse.org/Evolving_Java-based_APIs
> >> https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html
> >>
> >>
> >> Anthony
> >>
> >>
> >> > On May 14, 2019, at 12:45 PM, Dan Smith  wrote:
> >> >
> >> > Sounds good! Yeah, this is a bit of an interesting case where code
> will
> >> > still compile against the new API without change, but I think
> >> maintaining
> >> > compatibility of the binary API is also important. Thanks for looking
> >> into
> >> > it.
> >> >
> >> > -Dan
> >> >
> >> > On Tue, May 14, 2019 at 11:22 AM Ryan McMahon 
> >> wrote:
> >> >
> >> >> Hi all,
> >> >>
> >> >> This is my mistake - it was a misunderstanding of what constitutes a
> >> >> breaking public API change.  If a client app were to recompile using
> >> the
> >> >> new bits with the new method signature, there wouldn't be an issue
> >> because
> >> >> the new signature would work with 0 varargs.  The problem arises when
> >> you
> >> >> hot swap the geode dependencies for that client app without
> >> recompiling, as
> >> >> the bytecode no longer matches.  Since we do support the hot swap use
> >> case
> >> >> for CVEs etc, I see now this is considered a breaking API change.
> >> >>
> >> >> I'll change this to use a method overload instead, it should be a
> >> pretty
> >> >> simple fix.  Luckily, this issue didn't make it into any Geode
> >> releases.
> >> >>
> >> >> Thanks,
> >> >> Ryan
> >> >>
> >> >>
> >> >> On Tue, May 14, 2019 at 10:47 AM Udo Kohlmeyer 
> wrote:
> >> >>
> >> >>> How did this slip the review process that the signature of a public
> >> API
> >> >>> was changed?
> >> >>>
> >> >>> Well done Gester for finding this!!
> >> >>>
> >> >>> --Udo
> >> >>>
> >> >>> On 5/14/19 10:40, Dan Smith wrote:
> >>  I think the changes for GEODE-6327 broke backwards compatibility in
> >>  JSONFormatter with the change from fromJSON(String jsonString) to
> >>  fromJSON(String jsonString, String... identityFields)
> >> 
> >>  Adding an additional varargs parameter to the method breaks code
> that
> >> >> was
> >>  compiled against the non-varargs version. I think we need to
> overload
> >> >> the
> >>  method instead.
> >> 
> >>  Thanks to Gester for discovering this with his test!
> >> 
> >>  -Dan
> >> 
> >> >>>
> >> >>
> >>
> >>
>

Re: Gradle build broken?

[Jared Stewart]

You might also try ./gradlew build --refresh-dependencies if this happens
again.

On Wed, Jan 23, 2019 at 9:38 AM Kirk Lund  wrote:

> I saw the same problem a few weeks ago. I ended up deleting the directories
> in my .m2 repository and rebuilding. That seemed to fix it.
>
> The cause seems to have something to do with that log4j core tests jar, but
> I'm not sure why our build would be looking for the corresponding sources
> jar. If anyone knows what would cause this, let me know!
>
> On Tue, Jan 22, 2019 at 5:49 PM Galen O'Sullivan 
> wrote:
>
> > I'm getting the following failure building Geode on the latest develop.
> > I've tried `rm -r .gradle ~/.gradle`, to no avail. Any ideas?
> >
> > Thanks,
> > Galen
> >
> > ---
> >
> > ./gradlew dev
> > > Task :geode-core:compileIntegrationTestJava FAILED
> >
> > FAILURE: Build failed with an exception.
> >
> > * What went wrong:
> > Execution failed for task ':geode-core:compileIntegrationTestJava'.
> > > Could not resolve all files for configuration
> > ':geode-core:integrationTestCompileClasspath'.
> >> Could not find log4j-core-tests.jar
> > (org.apache.logging.log4j:log4j-core:2.11.1).
> >  Searched in the following locations:
> >
> >
> >
> file:/Users/gosullivan/.m2/repository/org/apache/logging/log4j/log4j-core/2.11.1/log4j-core-2.11.1-tests.jar
> >> Could not find log4j-core-test-sources.jar
> > (org.apache.logging.log4j:log4j-core:2.11.1).
> >  Searched in the following locations:
> >
> >
> >
> file:/Users/gosullivan/.m2/repository/org/apache/logging/log4j/log4j-core/2.11.1/log4j-core-2.11.1-test-sources.jar
> >
> > * Try:
> > Run with --stacktrace option to get the stack trace. Run with --info or
> > --debug option to get more log output. Run with --scan to get full
> > insights.
> >
> > * Get more help at
> >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__help.gradle.org=DwIBaQ=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw=oz-4MoNONnm7XFd0NWMb3g=xgO6NyJSiftex8QXD5QJ4pgduD0C7W-ovrychCh4GmU=ajS6jzRPUeYjdT-R2p1HDX_9A_660FRZjsJ4cRMD-_Y=
> >
> > Deprecated Gradle features were used in this build, making it
> incompatible
> > with Gradle 6.0.
> > Use '--warning-mode all' to show the individual deprecation warnings.
> > See
> >
> >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.gradle.org_5.0_userguide_command-5Fline-5Finterface.html-23sec-3Acommand-5Fline-5Fwarnings=DwIBaQ=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw=oz-4MoNONnm7XFd0NWMb3g=xgO6NyJSiftex8QXD5QJ4pgduD0C7W-ovrychCh4GmU=ZeKKsj9s-7D4ccqAPBX7hJddwZPlMHkGll8cCJayDbQ=
> >
> > BUILD FAILED in 24s
> >
>

Re: [VOTE] Time-based release schedule for minor releases

[Jared Stewart]

To my mind, one of the core reasons for SemVer is to communicate the level
of estimated risk to users when taking an update.  It seems to me that the
amount of code change included in a quarterly release will always introduce
more risk than a single narrowly-targeted fix for a CVE (like those that we
have previously released in patch versions).  Moreover, the introduction of
"substantial new functionality or improvments" is a *sufficient* condition
for incrementing minor versions, but is not a *necessary* condition.  For
these reasons, I vote +1 to aim for quarterly at-least-minor releases.

On Wed, Oct 10, 2018 at 11:17 AM Bruce Schuchardt 
wrote:

> Practically speaking I think I agree with Anthony.  I'm changing my vote
> to +0 but I do still feel that we're not going to be following the
> spirit of our major/minor/patch definitions.  A quarterly release might
> be a Minor release or it might be a Patch release, depending on whether
> there are actually any functional changes.  We should change those
> definitions to match what we're actually doing.
>
>
> On 10/10/18 9:35 AM, Anthony Baker wrote:
> > Practically speaking, a quarterly release cycle means there’s *always*
> some feature addition or improvement included in the release.  That’s why I
> agree with the suggestion of a release cadence based on minor version
> bumps.  See [1] for the outcome of prior discussions on SemVer.
> >
> > Anthony
> >
> > [1]
> https://cwiki.apache.org/confluence/display/GEODE/Versioning+and+Branching
> >
> >
> >> On Oct 10, 2018, at 8:44 AM, Ryan McMahon 
> wrote:
> >>
> >> I’m with Sai that it seems like we need to clear up our definitions of
> >> minor versus patch releases.  The referenced SemVer definition indicates
> >> that any backwards compatible bug fix qualifies for a patch release.
> But
> >> it was stated earlier that only security-related or critidal bug fixes
> >> justify a patch release.  I personally prefer SemVer’s definition, but
> it’s
> >> up for debate.
> >>
> >> Perhaps we can do 3-month release cycles, and determine whether the
> release
> >> would be patch or minor based on the changes added since the last
> release
> >> (bug fixes vs new functionality).
> >>
> >> Ryan
>
>

Re: Has anyone else been getting gradle "Unable to start the daemon process."

[Jared Stewart]

You might be running into this: https://stackoverflow.com/a/52510288

On Mon, Oct 1, 2018 at 2:03 PM Patrick Rhomberg 
wrote:

> Hello all.
>
>   I've been getting some strange failures lately, with the output given
> below.  I have noticed that creating a `gradle wrapper` fixes these
> issues.  However, since the gradle wrapper is part of the git tree, I was
> curious to know if anyone else was having this problem.
>   If so, we must've committed a bad jar somehow.  If not, I'll continue to
> try to suss out how I've dirtied my local environment.
>
> Imagination is Change.
> ~Patrick
>
> Gradle output:
>
> |develop → origin ✓| → ./gradlew dev
> Starting a Gradle Daemon (subsequent builds will be faster)
>
> FAILURE: Build failed with an exception.
>
> * What went wrong:
> Unable to start the daemon process.
> This problem might be caused by incorrect configuration of the daemon.
> For example, an unrecognized jvm option is used.
> Please refer to the user guide chapter on the daemon at
> https://docs.gradle.org/4.10.1/userguide/gradle_daemon.html
> Please read the following process output to find out more:
> ---
>
> FAILURE: Build failed with an exception.
>
> * What went wrong:
> Could not create service of type ClassLoaderRegistry using
> GlobalScopeServices.createClassLoaderRegistry().
>
> * Try:
> Run with --stacktrace option to get the stack trace. Run with --info or
> --debug option to get more log output. Run with --scan to get full
> insights.
>
> * Get more help at https://help.gradle.org
>
>
> * Try:
> Run with --stacktrace option to get the stack trace. Run with --info or
> --debug option to get more log output. Run with --scan to get full
> insights.
>
> * Get more help at https://help.gradle.org
>

Re: [VOTE] Apache Geode 1.5.0.RC2

[Jared Stewart]

Out of curiosity, what was Pulkit's suggestion? I don't see it in this
thread.

On Thu, Apr 5, 2018, 8:32 AM Alexander Murmann  wrote:

> I am very much in favor of Pulkit's suggestion. We've previously discussed
> using something like
> https://github.com/nebula-plugins/gradle-dependency-lock-plugin. This
> would
> make a process like Pulkit describes very easy. We could easily be on the
> latest versions that are known to work and at the same time capture which
> dependencies cannot easily be upgraded. This would safe lots of manual work
> and also provide greater transparency to us into where actual human effort
> is required to get back catch up with latest.
>
> On Thu, Apr 5, 2018 at 8:12 AM, Anthony Baker  wrote:
>
> > I created https://issues.apache.org/jira/browse/GEODE-5001 for this.
> >
> > Anthony
> >
> >
> > > On Apr 4, 2018, at 5:39 PM, John Blum  wrote:
> > >
> > > +0
> > >
> > >
> > > The Apache Geode *Log4j* dependency version *2.8.2* is or will cause
> > > significant issues for apps, and in particular *Spring Boot* 2.0 apps.
> > >
> > > This Geode Log4j version is already quite dated as *Log4j 2.11.0* is
> now
> > > already available [1] and *Spring Boot* 2.0 pulls in *Log4j 2.10.0*
> [2].
> >
> >
>

Re: [DISCUSS]: Tests requiring external services

[Jared Stewart]

Just FYI, the reason that :acceptanceTest is currently only a target of
precheckin is https://issues.apache.org/jira/browse/GEODE-3296

For the full details, see this thread on the Gradle Forums:
https://discuss.gradle.org/t/test-task-with-forkevery-1-and-includecategories-performs-poorly/23401


On Wed, Apr 4, 2018 at 9:56 AM, Patrick Rhomberg 
wrote:

> +1.  AcceptanceTest seems fittings, although...
>
> That test category was created with the focus on tests that run gfsh
> scripts via the GfshRule.  Because the GfshRule uses the built jar and
> actually launches gfsh to run its tests, all current AcceptanceTests exist
> in geode-assembly.  Perhaps as an oversight, only
> :geode-assembly:acceptanceTest is a target of the precheckin task.
>
> If we want to expand the scope of the AcceptanceTest tag, we'll need to go
> update the gradle to make sure these tests get picked up.
>
> On Wed, Apr 4, 2018 at 9:39 AM, Kirk Lund  wrote:
>
> > +1 to using AcceptanceTest category for the end-to-end JDBC connector
> > service tests
> >
> > On Wed, Apr 4, 2018 at 8:41 AM, Nick Reich  wrote:
> >
> > > Using AcceptanceTest category seems like a good solution at the moment
> to
> > > me.
> > >
> > > On Tue, Apr 3, 2018 at 4:29 PM, Sean Goller 
> wrote:
> > >
> > > > I'm actually fine with putting it in AcceptanceTest for now.
> > > >
> > > > Ideally I'd like to see something like JDBC connection strings that
> > could
> > > > be passed in as properties via the command-line, and if they're not
> > > present
> > > > the relevant tests don't get run. That way the entity running the
> tests
> > > can
> > > > decide the best way to enable those tests.
> > > >
> > > > On Tue, Apr 3, 2018 at 4:11 PM, Jens Deppe 
> wrote:
> > > >
> > > > > I'm in favor of using docker for test isolation. We already have an
> > > > > 'AcceptanceTest' category which you might consider using instead of
> > > > > creating yet another category.
> > > > >
> > > > > --Jens
> > > > >
> > > > > On Tue, Apr 3, 2018 at 2:02 PM, Nick Reich 
> > wrote:
> > > > >
> > > > > > Team,
> > > > > >
> > > > > > As part of validating the new JDBC connector for Geode, we have a
> > > need
> > > > > for
> > > > > > tests that involving connecting to specific databases (like MySQL
> > and
> > > > > > Postgres) to validate proper function with those databases. Since
> > > these
> > > > > > tests require connecting to outside services, unlike existing
> Geode
> > > > > tests,
> > > > > > we are seeking suggestions on how to best incorporate such tests
> > into
> > > > > > Geode. The approach we have taken so far is to utilize Docker
> (and
> > > > Docker
> > > > > > Compose) to take care of spinning up our external services for
> the
> > > > > duration
> > > > > > of the tests. This, however requires that Docker and Docker
> Compose
> > > be
> > > > > > installed on any machine that the tests are run on. Additionally,
> > the
> > > > > > Concourse pipeline for validating develop is incompatible with
> use
> > of
> > > > > > Docker for distributed tests, due to the way they are already
> being
> > > run
> > > > > > within Docker containers of their own (it seems possible to make
> it
> > > > work,
> > > > > > but would add overhead to all tests and would be a challenge to
> > > > > implement).
> > > > > >
> > > > > > To address these issues, we are considering having these tests
> run
> > > > under
> > > > > a
> > > > > > new task, such as "serviceTest" (instead of IntegrationTest or
> > > > > > distributedTest). That way, developers could run all other tests
> > > > normally
> > > > > > on their machines, only requiring Docker and Docker Compose if
> they
> > > > wish
> > > > > to
> > > > > > run these specific tests. This would also allow them to be their
> > own
> > > > task
> > > > > > in Concourse, eliminating the issues that plague integrating
> these
> > > > tests
> > > > > > there.
> > > > > >
> > > > > > Are there other ideas on how to manage these tests or concerns
> with
> > > the
> > > > > > proposed approach?
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [Discussion] Improving Spotless to be Even More Beautiful

[Jared Stewart]

+1 to all of the above

On Mon, Mar 19, 2018 at 2:51 PM, Patrick Rhomberg 
wrote:

> Hello all!
>
>  I'm making another pass at patching up some of our smellier broken
> windows, to mix metaphors.  To that end, there are a few things I'd like to
> add to spotless to more closely enforce adherence to our own style guide.
>
> I'd like to do the following to our spotless:
> - Bump spotless to the newest version, enabling some of the features below
> - Have spotless only run spotless on files that have changed (Yay, slightly
> faster builds!)
> - Have spotless perform the following spotless tasks:
> --- Remove unused imports.
> --- Prohibit wildcard imports, which may require manual expansion by the
> developer.  (But, in fairness, you shouldn't be using wildcard imports
> anyway.)
> --- Remove trivial javadoc stubs that are implicit in the method signature
> (e.g., "@param p" with no description of what p actually is).
> --- Remove empty javadocs and block comments.
>
> This will again touch a great many files, so it should be targeted for
> immediately following a release, perhaps the impending 1.5.  It can pretty
> easily be exploded into several commits across each subproject and task for
> ease of review.
>
> Thoughts?
>
> Imagination is Change.
> ~Patrick Rhomberg
>

Re: Debugging dunit in intellij now fails?

[Jared Stewart]

That looks like you have already have a process running on the port that
the debugger is trying to connect on.   I'd try to netstat -an and kill any
process bound to that port.

On Tue, Mar 6, 2018 at 1:06 PM, Kirk Lund  wrote:

> Anyone else see this when attempting to use intellij debugger on dunit??
>
> [locator] ERROR: transport error 202: bind failed: Address already in use
> [locator] ERROR: JDWP Transport dt_socket failed to initialize,
> TRANSPORT_INIT(510)
> [locator] JDWP exit error AGENT_ERROR_TRANSPORT_INIT(197): No transports
> initialized [debugInit.c:750]
>

Re: [VOTE] Apache Geode release - 1.3.0 RC4

[Jared Stewart]

+1

Built from source and performed basic operations against a small cluster.

> On Oct 26, 2017, at 11:54 AM, Swapnil Bawaskar  wrote:
> 
> This is the fourth release candidate for Apache Geode, version 1.3.0.
> Thanks to all the community members for their contributions to this
> release!
> 
> *** Please download, test and vote by Sunday, October 29, 1200 hrs
> US Pacific. ***
> 
> It fixes 376 issues. release notes can be found at:
> https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12318420=12340669
> 
> Note that we are voting upon the source tags:  rel/v1.3.0.RC4
> https://github.com/apache/geode/tree/rel/v1.3.0.RC4
> https://github.com/apache/geode-examples/tree/rel/v1.3.0.RC4
> 
> Commit ID:
> 59f2a73d108101744ed7b2d88e8d1c948d19781c (geode)
> 4ff8f8eafd0927888e711ee45d283ab07d345000   (geode-examples)
> 
> Source and binary files:
> https://dist.apache.org/repos/dist/dev/geode/1.3.0.RC4
> 
> Maven staging repo:
> https://repository.apache.org/content/repositories/orgapachegeode-1035
> 
> 
> Geode's KEYS file containing PGP keys we use to sign the release:
> https://github.com/apache/geode/blob/develop/KEYS
> 
> Release Signed with Key: pub 4096R/18F902DB 2016-04-07
> Fingerprint: E1B1 ABE3 4753 E7BA 8097 4285 8F8F 2BCC 18F9 02DB

Re: [Discuss] CliStrings

[Jared Stewart]

> On Oct 20, 2017, at 1:59 PM, Dan Smith  wrote:
> 
> +1 for removing it.
> 
> I do think it would be nice if we add localization in the future. But
> I don't really like the idea of leaving stuff in our code just in case
> we decide to implement a feature in the future - we might not even
> want what we left in there! In this case even with localization we
> might want to move these constants into their specific command classes
> anyway - the constant might just look up the localized string instead
> of being hardcoded.
> 
> -Dan

Totally agree with this reasoning, I’m a firm believer in the YAGNI principle. 
(https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it 
)

- Jared

[Discuss] CliStrings

[Jared Stewart]

I wanted to kick off a discussion about the usage of CliStrings.  For those 
unfamiliar, it’s a java class that contains about ~3000 lines of String 
constants and has a javadoc explaining that it is an attempt at i18n 
localization.  Does anyone know if this localization is actually implemented in 
practice?  

If not, I would like suggest that we try to move away from this pattern going 
forward.  We have ended up with many constants in CliStrings like this: 
CliStrings.CREATE_REGION__MSG__ONLY_ONE_OF_REGIONSHORTCUT_AND_USEATTRIBUESFROM_CAN_BE_SPECIFIED
The constant is only used in CreateRegionCommand, so I would be happier to see 
it as a member of CreateRegionCommand (where there would be no need for the 
unwieldy "CREATE_REGION__MSG__” prefix) unless there is localization being done 
which I am unaware of.

Thoughts?

Thanks,
Jared

Re: [Discuss] Geode gradle plugin

[Jared Stewart]

Yeah, the Plugin Portal is great!  Not only is it easy to publish to, it also 
makes the plugin easier to add for users. If you publish to the Portal, a user 
only needs to add a plugins block: 

plugins {
id 'com.jfrog.bintray' version '0.4.1'
}

Whereas if you publish to Bintray, JCenter, Maven Central, etc, a user must 
also add a build script dependency: 

buildscript {
repositories {
jcenter()
}
dependencies {
classpath "com.jfrog.bintray.gradle:gradle-bintray-plugin:0.4.1"
}
}

apply plugin: "com.jfrog.bintray"

I haven’t registered yet, since I wasn’t sure what email address to use for 
registration or where to store our API key.  How do we manage the API key for 
publishing Geode releases to Maven Central?

Thanks,
Jared

> On Oct 12, 2017, at 9:31 AM, Anthony Baker <aba...@pivotal.io> wrote:
> 
> Have you looked at https://plugins.gradle.org/docs/submit 
> <https://plugins.gradle.org/docs/submit> 
> <https://plugins.gradle.org/docs/submit 
> <https://plugins.gradle.org/docs/submit>> ?
> 
> Anthony
> 
> 
>> On Oct 11, 2017, at 7:56 PM, Jared Stewart <jstew...@pivotal.io 
>> <mailto:jstew...@pivotal.io>> wrote:
>> 
>> Hi all,
>> 
>> I've been working on a Gradle plugin to make it easier to write integration 
>> tests for applications that use Apache Geode, and would like to discuss 
>> where it should reside.  
>> 
>> To give some background, we have a JUnit Rule called GfshRule that lets you 
>> write tests that execute gfsh commands. The rule is exported in geode-junit, 
>> so developers of Geode applications can use the rule to start up a transient 
>> cluster for their own integration testing, among other things.  
>> However, the rule relies on having the GEODE_HOME environment variable set 
>> to an existing installation of Geode, which can be problematic for CI 
>> environments or tests running inside of containers.
>> 
>> The Gradle plugin will add a task that downloads a distribution of Geode via 
>> Maven, unzips it into build/install/apache-geode, and sets the proper 
>> environment variable for any tests that are run through Gradle. 
>> 
>> It would be nice to avoid having the releases of Geode and the plugin tied 
>> together, so I thought I would suggest having a separate repository for the 
>> plugin (similar to geode-examples).  Does anyone have thoughts on whether 
>> that's the correct place for the plugin to live, or on what it should be 
>> called?  geode-integration-gradle-plugin?
>> 
>> Any feedback is appreciated,
>> - Jared
>> 
>> P.S. If you want to take a look at the work in progress, it's been pushed 
>> here for now: https://github.com/jaredjstewart/geode-integration-plugin 
>> <https://github.com/jaredjstewart/geode-integration-plugin><https://github.com/jaredjstewart/geode-integration-plugin
>>  <https://github.com/jaredjstewart/geode-integration-plugin>>

[Discuss] Geode gradle plugin

[Jared Stewart]

Hi all,

I've been working on a Gradle plugin to make it easier to write integration 
tests for applications that use Apache Geode, and would like to discuss where 
it should reside.  

To give some background, we have a JUnit Rule called GfshRule that lets you 
write tests that execute gfsh commands. The rule is exported in geode-junit, so 
developers of Geode applications can use the rule to start up a transient 
cluster for their own integration testing, among other things.  
However, the rule relies on having the GEODE_HOME environment variable set to 
an existing installation of Geode, which can be problematic for CI environments 
or tests running inside of containers.

The Gradle plugin will add a task that downloads a distribution of Geode via 
Maven, unzips it into build/install/apache-geode, and sets the proper 
environment variable for any tests that are run through Gradle. 

It would be nice to avoid having the releases of Geode and the plugin tied 
together, so I thought I would suggest having a separate repository for the 
plugin (similar to geode-examples).  Does anyone have thoughts on whether 
that's the correct place for the plugin to live, or on what it should be 
called?  geode-integration-gradle-plugin?

Any feedback is appreciated,
- Jared

P.S. If you want to take a look at the work in progress, it's been pushed here 
for now: https://github.com/jaredjstewart/geode-integration-plugin 

Re: [DISCUSS] CI improvements

[Jared Stewart]

+1 I think this will be a huge improvement to the reliability of our test 
infrastructure.

- Jared

> On Oct 6, 2017, at 9:26 AM, Kirk Lund  wrote:
> 
> +1 no thoughts other than make it so!
> 
> On Fri, Oct 6, 2017 at 7:08 AM, Anthony Baker  wrote:
> 
>> Hi all,
>> 
>> I’d like to propose the following that we switch our continuous
>> integration (CI) system from Jenkins [1] to Concourse [2].  I suggest
>> this because we continue to experience a significant number of
>> environmental-related test failures.
>> 
>> These issues include CPU interference from other Jenkins jobs on the
>> same host, running out of disk space, port conflicts, and other
>> gremlins.  The net effect is that we are only getting 1-2 successful
>> builds per month.  Certainly not all test failures can be traced back
>> to environmental issues.  However, internal testing on isolated VM’s
>> shows a combined success rate of about 3X higher compared to ASF
>> Jenkins for the same tests.  This is still definitely NotAwesome, but
>> removing environmental factors will let us focus on stabilizing flaky
>> tests.
>> 
>> Concourse is an Apache-licensed open source CI system based on
>> pipelines.  The pipelines are defined in a YML file containing job
>> definitions—inputs, outputs, resources, and tasks.  A task is simply a
>> bash script that returns 0/1 for success/failure.  A web UI displays
>> build status.  Importantly, each job runs inside an isolated
>> container.  The containers are load-balanced across a pool of workers.
>> For an example of a build pipeline, see [3] for the pipeline used to
>> build concourse itself.
>> 
>> A Concourse environment is deployed and managed in cloud environments
>> through bosh [4].  Pivotal has agreed to donate AWS and/or GCP compute
>> and storage resources as well as manage the infrastructure.  These
>> project resources would be available for use by all committers and
>> community members regardless of corporate affiliations.  Note that
>> AFAIK there is no explicit requirement to host CI on ASF
>> infrastructure—unlike for critical project resources such as source
>> code, mailing lists, and issue tracking.
>> 
>> The source for the pipeline and job scripts would reside within the
>> geode-* repos.  Geode committers would be able to modify those, same
>> as with our .travis.yml scripts.  All test results and build artifacts
>> would be publicly viewable just like with our Jenkins build output
>> today.  Requests for admin assistance would go through the dev@geode
>> mailing list.
>> 
>> Thoughts?  As a first step we could run both CI systems side-by-side
>> and see how the Concourse approach works for our project.
>> 
>> Thanks,
>> Anthony
>> 
>> 
>> [1] https://builds.apache.org/job/Geode-nightly/
>> [2] https://concourse.ci
>> [3] https://ci.concourse.ci
>> [4] https://bosh.io
>> 

Re: Rebase and squash before merging PRs

[Jared Stewart]

Does anyone happen to know if “squash and merge” also does a rebase or not? 
I’ve been hesitant to use that button since I’m not sure what exact sequence of 
git commands it corresponds to.

> On Oct 5, 2017, at 3:59 PM, Jason Huynh <jhu...@pivotal.io> wrote:
> 
> I think we can also use "squash and merge" if wanting to squash commits
> before merging.  This would allow you not to have to force push every time.
> 
> On Thu, Oct 5, 2017 at 3:15 PM Jinmei Liao <jil...@pivotal.io> wrote:
> 
>> On the PR UI page, you can do that by pull down the the menu when you are
>> ready to merge. Remember to use "Rebase and merge".
>> 
>> 
>> ​
>> 
>> Not sure if this is useful to everyone, but when I push a subsequent commit 
>> to my feature branch, I always use "force push", so that it's only one 
>> commit I need to rebase to develop.
>> 
>> 
>> On Thu, Oct 5, 2017 at 3:00 PM, Jared Stewart <jstew...@pivotal.io> wrote:
>> 
>>> I’ve been seeing a lot more merge commits on develop since we moved to
>>> Gitbox.  Just wanted to give everyone a friendly reminder to please rebase
>>> before merging to keep our git history tidy and readable.
>>> 
>>> Thanks,
>>> Jared
>> 
>> 
>> 
>> 
>> --
>> Cheers
>> 
>> Jinmei
>> 

Rebase and squash before merging PRs

[Jared Stewart]

I’ve been seeing a lot more merge commits on develop since we moved to Gitbox.  
Just wanted to give everyone a friendly reminder to please rebase before 
merging to keep our git history tidy and readable.

Thanks,
Jared

Re: ExportConfigCommandDUnitTest

[Jared Stewart]

That test uses AvailablePortFinder to choose a random available port.  It’s 
possible that you hit a race condition in between the time port was chosen and 
the time it the server attempted to bind to it.  

> On Oct 4, 2017, at 5:20 PM, Kirk Lund <kl...@apache.org> wrote:
> 
> It passes when I run it in IntelliJ. The cause was:
> 
> Caused by: java.net.BindException: Address already in use
> 
> So, I'm assuming the test tried to use a port that was in use. Is the test
> using a random port or a default port?
> 
> On Wed, Oct 4, 2017 at 5:14 PM, Jared Stewart <jstew...@pivotal.io> wrote:
> 
>> I haven’t seen that test fail before.  If you rerun the precheckin does it
>> still fail?
>> 
>> 
>>> On Oct 4, 2017, at 4:55 PM, Kirk Lund <kl...@apache.org> wrote:
>>> 
>>> ExportConfigCommandDUnitTest failed in my latest precheckin but my
>> changes
>>> don't involve the code this test is testing. Did someone commit break
>> this
>>> test or is it failing intermittently?
>>> 
>>> Thanks,
>>> Kirk
>>> 
>>> :geode-web:distributedTest
>>> 
>>> org.apache.geode.management.internal.cli.commands.
>> ExportConfigCommandDUnitTest
>>>> testExportConfig(true) [0] FAILED
>>>   java.lang.AssertionError: Suspicious strings were written to the log
>>> during this run.
>>>   Fix the strings or use IgnoredException.addIgnoredException to
>> ignore.
>>>   
>> ---
>>>   Found suspect string in log4j at line 336
>>> 
>>>   [error 2017/10/04 23:34:54.964 UTC 
>>> tid=0x1b] Jmx manager could not be started because HTTP service failed to
>>> start
>>>   org.apache.geode.management.ManagementException: HTTP service failed
>> to
>>> start
>>>   at
>>> org.apache.geode.management.internal.ManagementAgent.startHttpService(
>> ManagementAgent.java:314)
>>>   at
>>> org.apache.geode.management.internal.ManagementAgent.
>> startAgent(ManagementAgent.java:143)
>>>   at
>>> org.apache.geode.management.internal.SystemManagementService.
>> startManager(SystemManagementService.java:427)
>>>   at
>>> org.apache.geode.management.internal.beans.ManagementAdapter.
>> handleCacheCreation(ManagementAdapter.java:173)
>>>   at
>>> org.apache.geode.management.internal.beans.ManagementListener.
>> handleEvent(ManagementListener.java:106)
>>>   at
>>> org.apache.geode.distributed.internal.InternalDistributedSystem.
>> notifyResourceEventListeners(InternalDistributedSystem.java:2198)
>>>   at
>>> org.apache.geode.distributed.internal.InternalDistributedSystem.
>> handleResourceEvent(InternalDistributedSystem.java:585)
>>>   at
>>> org.apache.geode.internal.cache.GemFireCacheImpl.
>> initialize(GemFireCacheImpl.java:1207)
>>>   at
>>> org.apache.geode.internal.cache.GemFireCacheImpl.
>> basicCreate(GemFireCacheImpl.java:768)
>>>   at
>>> org.apache.geode.internal.cache.GemFireCacheImpl.create(
>> GemFireCacheImpl.java:754)
>>>   at org.apache.geode.cache.CacheFactory.create(CacheFactory.java:175)
>>>   at org.apache.geode.cache.CacheFactory.create(CacheFactory.java:222)
>>>   at
>>> org.apache.geode.test.junit.rules.ServerStarterRule.
>> startServer(ServerStarterRule.java:147)
>>>   at
>>> org.apache.geode.test.junit.rules.ServerStarterRule.
>> before(ServerStarterRule.java:76)
>>>   at
>>> org.apache.geode.test.dunit.rules.LocatorServerStartupRule.lambda$
>> startServerAsEmbededLocator$207b7110$1(LocatorServerStartupRule.java:230)
>>>   at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>>>   at
>>> sun.reflect.NativeMethodAccessorImpl.invoke(
>> NativeMethodAccessorImpl.java:62)
>>>   at
>>> sun.reflect.DelegatingMethodAccessorImpl.invoke(
>> DelegatingMethodAccessorImpl.java:43)
>>>   at java.lang.reflect.Method.invoke(Method.java:498)
>>>   at hydra.MethExecutor.executeObject(MethExecutor.java:245)
>>>   at
>>> org.apache.geode.test.dunit.standalone.RemoteDUnitVM.
>> executeMethodOnObject(RemoteDUnitVM.java:70)
>>>   at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>>>   at
>>> sun.reflect.NativeMethodAccessorImpl.invoke(
>> NativeMethodAccessorImpl.java:62)
>>>   at
>>> sun.reflect.DelegatingMethodAccessorImpl.invoke(
>> DelegatingMethodAccessorImpl.

Re: How gitbox works

[Jared Stewart]

Https://github.com/apache is where I had to accept the invite and get the
third box green. I didn't seem to get an email invite either.

- Jared

On Sep 18, 2017 12:51 PM, "Kirk Lund"  wrote:

If I navigate to https://github.com/apache, I see the following at the top
with a button to "View invitation" and accept:

asfgit  invited you to join the The Apache
Software Foundation organization on Mar 7, 2016.

Is this the expected invitation that I need to accept?! Apache never sent
me any email. I even tried removing and re-adding my github account to my
Apache Profile. This apache-gitbox integration really isn't ready for
prime-time usage is it.

Thanks,
Kirk


On Mon, Sep 18, 2017 at 12:43 PM, Kirk Lund  wrote:

> #3 is not GREEN, however I already have my github account listed on my
> Apache account:
>
> Your GitHub Username: kirklund
>
> So, what's missing or do I need to contact INFRA?
>
> Thanks,
> Kirk
>
> On Mon, Sep 18, 2017 at 12:38 PM, Mark Bretl  wrote:
>
>> Hi Kirk,
>>
>> I would go to the GitBox account linking utility page at
>> https://gitbox.apache.org/setup/ and verify all three steps are green. If
>> they are green, then probably need to contact INFRA to figure out the
>> issue
>> with your account.
>>
>> --Mark
>>
>> On Mon, Sep 18, 2017 at 10:13 AM, Kirk Lund  wrote:
>>
>> > PS: If I had known the switchover to github would be this messy I would
>> > have voted against it.
>> >
>> > On Mon, Sep 18, 2017 at 10:12 AM, Kirk Lund  wrote:
>> >
>> >> Yeah, I followed all of the directions shared by Jens and Bruce. I
also
>> >> had my github account added to my Apache account for the last 2-3
>> years.
>> >> Apparently the switchover to gitbox removed by commit permission.
>> >>
>> >> /Users/klund/dev/geode [525]$ git push
>> >> ERROR: Permission to apache/geode.git denied to kirklund.
>> >> fatal: Could not read from remote repository.
>> >>
>> >> Please make sure you have the correct access rights
>> >> and the repository exists.
>> >>
>> >> I also don't have the "Merge" button available to me when viewing
>> Apache
>> >> Geode pull requests.
>> >>
>> >> Anyone know who to contact to restore it?
>> >>
>> >> Thanks,
>> >> Kirk
>> >>
>> >> On Mon, Sep 18, 2017 at 10:05 AM, Kirk Lund  wrote:
>> >>
>> >>> Apparently I no longer have commit access. Anyone know who/how I get
>> >>> access restored?
>> >>>
>> >>> Thanks,
>> >>> Kirk
>> >>>
>> >>> On Mon, Sep 18, 2017 at 9:49 AM, Jacob Barrett 
>> >>> wrote:
>> >>>
>>  GitBox is just a fancy name Apache has given to their service that
>>  allows us to use GitHub as the source of authority for our Git repo.
>>  Everything is done on GitHub using the GitHub processes.
>> 
>>  Anyone with write access to GitHub can merge a PR. The PR can be
>> merged
>>  from the PR GitHub website.
>> 
>>  -Jake
>> 
>> 
>>  > On Sep 18, 2017, at 9:43 AM, Kirk Lund  wrote:
>>  >
>>  > I've searched and not found a good summary on how gitbox works.
>>  Anyone know
>>  > of a good resource?
>>  >
>>  > The kinds of questions I have are: if other devs approve my PR, do
>> I
>>  still
>>  > need to find another dev to commit for me or can I commit my own
PR
>>  if has
>>  > been approved?
>>  >
>>  > Thanks,
>>  > Kirk
>> 
>> >>>
>> >>>
>> >>
>> >
>>
>
>

Re: Request: Build flag to skip download of previous Geode versions

[Jared Stewart]

I think this might do it: 

./gradlew build -x :geode-old-versions:compileJava -x 
:geode-old-versions:verifyGeodetest120 -x 
:geode-old-versions:downloadZipFiletest120 -x 
:geode-old-versions:downloadAndUnzipFiletest120 

> On Sep 18, 2017, at 9:46 AM, Kirk Lund  wrote:
> 
> I filed GEODE-3639: "Request: build flag to skip download of previous Geode
> versions" for this request.
> 
> Thanks,
> Kirk
> 
> On Mon, Sep 18, 2017 at 9:40 AM, Kirk Lund  wrote:
> 
>> Turns out the 13% has nothing to do with the download, but I still think a
>> build flag to skip this would be a good idea.
>> 
>> On Mon, Sep 18, 2017 at 9:38 AM, Kirk Lund  wrote:
>> 
>>> So my build has been stuck on our favorite download for the last 10
>>> minutes and is only up to 13% which tells me it could take 20+ minutes this
>>> morning...
>>> 
>>> Download https://www.apache.org/dyn/closer.cgi?action=download
>>> me=geode/1.2.0/apache-geode-1.2.0.tar.gz
>>> <=> 13% EXECUTING
 :geode-old-versions:downloadZipFiletest120 > 38.86 MB/87.14 MB
>>> downloaded
>>> 
>>> What's I'd really like is a build flag to SKIP this step. Is anyone's
>>> Gradle-fu good enough to add in a build flag that would accomplish this?
>>> 
>>> Thanks,
>>> Kirk
>>> 
>> 
>> 

Re: [DISCUSS] authorizing function execution

[Jared Stewart]

There is nothing to prevent code in a function that's executing on a server 
from reaching into internal classes and bypassing the public region APIs. I 
think a function's author should ultimately determine the permissions required 
to execute it.   

> On Sep 14, 2017, at 10:34 AM, Anilkumar Gingade <aging...@pivotal.io> wrote:
> 
> When a function is accessing/modifying region; the function will be doing
> so by region apis, don't we have credential check with region apis; if not
> can we add those checks here...instead of having it in the function...
> 
> -Anil.
> 
> 
> 
> On Wed, Sep 13, 2017 at 11:22 AM, Jared Stewart <jstew...@pivotal.io> wrote:
> 
>> After some more investigation into the implementation details, here is our
>> updated proposal to add to the Function interface:
>> 
>> default Collection 
>> getRequiredPermissions(Optional
>> onRegion) {
>>  return Collections.singletonList(ResourcePermissions.DATA_WRITE);
>> }
>> 
>> This method can be overridden by Function authors who want to require
>> permissions other than DATA:WRITE.. The onRegion parameter will be present
>> only when a Function is executed via FunctionService.onRegion, and is
>> intended to allow Function authors to require different permissions
>> depending on the Region which Function will be executed on.  We pass the
>> region name into this method rather than the full FunctionContext because
>> the latter would be much more expansive to implement.
>> 
>> Any feedback is appreciated.
>> 
>> Thanks,
>> Jared
>> 
>>> On Aug 17, 2017, at 1:42 AM, Swapnil Bawaskar <sbawas...@pivotal.io>
>> wrote:
>>> 
>>> Discuss fix for GEODE-2817
>>> <https://issues.apache.org/jira/browse/GEODE-2817>
>>> 
>>> Currently to execute a function, you will need "data:write" permission,
>> but
>>> it really depends on what the function is doing. For example, if a
>> function
>>> is just reading data, the function author might want users with DATA:READ
>>> permissions to execute the function. The two options mentioned in the
>>> ticket are:
>>> 
>>> 1) externalize SecurityService so that function author can use it in the
>>> function.execute code to check authorization.
>>> 2) add a method to function interface to tell the framework what
>> permission
>>> this function needs to execute, so that the framework will check the
>>> permission before executing the function.
>>> 
>>> I vote for #2 because, I think, a function author will be able to easily
>>> discover a method on the Function interface, rather than trying to look
>> for
>>> SecurityService.
>>> 
>>> I propose that we add the following new method to Function:
>>> 
>>> default public List requiredPermissions() {
>>>  // default DATA:WRITE
>>> }
>>> 
>>> In order to preserve existing behavior, the default required permission
>>> would be DATA:WRITE.
>> 
>> 

Re: [DISCUSS] authorizing function execution

[Jared Stewart]

After some more investigation into the implementation details, here is our 
updated proposal to add to the Function interface:

default Collection getRequiredPermissions(Optional 
onRegion) {
  return Collections.singletonList(ResourcePermissions.DATA_WRITE);
}

This method can be overridden by Function authors who want to require 
permissions other than DATA:WRITE.. The onRegion parameter will be present only 
when a Function is executed via FunctionService.onRegion, and is intended to 
allow Function authors to require different permissions depending on the Region 
which Function will be executed on.  We pass the region name into this method 
rather than the full FunctionContext because the latter would be much more 
expansive to implement. 

Any feedback is appreciated.

Thanks,
Jared

> On Aug 17, 2017, at 1:42 AM, Swapnil Bawaskar  wrote:
> 
> Discuss fix for GEODE-2817
> 
> 
> Currently to execute a function, you will need "data:write" permission, but
> it really depends on what the function is doing. For example, if a function
> is just reading data, the function author might want users with DATA:READ
> permissions to execute the function. The two options mentioned in the
> ticket are:
> 
> 1) externalize SecurityService so that function author can use it in the
> function.execute code to check authorization.
> 2) add a method to function interface to tell the framework what permission
> this function needs to execute, so that the framework will check the
> permission before executing the function.
> 
> I vote for #2 because, I think, a function author will be able to easily
> discover a method on the Function interface, rather than trying to look for
> SecurityService.
> 
> I propose that we add the following new method to Function:
> 
> default public List requiredPermissions() {
>   // default DATA:WRITE
> }
> 
> In order to preserve existing behavior, the default required permission
> would be DATA:WRITE.

Re: Flaky failures in Geode Nightly Build

[Jared Stewart]

I would prefer not to mark ExportLogsStatsOverHttpDUnitTest as flaky.  This is 
the first time we’ve seen it fail, and it looks like it failed due to 
insufficient cleanup from a previous test.  We’re working on finding the 
culprit test and adding the missing cleanup.  

(Or you can mark it flaky for now, and we can unmark it as flaky when we push a 
fix today/tomorrow.)

- Jared

> On Sep 13, 2017, at 9:34 AM, Kirk Lund  wrote:
> 
> We need to get more serious about getting the Geode Nightly Build back to
> consistent GREEN.
> 
> I want to mark these flickering tests as FlakyTest. Please let me know if
> you have a problem with me doing so:
> 
> 1) Jetty9CachingClientServerTest
> 2) ExportLogsStatsOverHttpDUnitTest
> 
> If you do have a problem with marking these as FlakyTest, please suggest
> how we can get the build GREEN.
> 
> Please remember that we are NOT supposed to ignore any test failures in the
> flakyTest target. Any failures in that target should be prioritized for
> fixing and de-flickering ASAP. This is much better than marking such tests
> with @Ignore.
> 
> Thanks,
> Kirk

Re: Review Request 62163: GEODE-3096: pulling in refactoring work on HttpOperationInvoker

[Jared Stewart]

> On Sept. 11, 2017, 4:54 p.m., Jared Stewart wrote:
> > geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/GfshStartLocatorLogTest.java
> > Line 36 (original), 33 (patched)
> > <https://reviews.apache.org/r/62163/diff/1/?file=1817628#file1817628line36>
> >
> > I think this probably ought to be `AcceptanceTest`.
> 
> Jinmei Liao wrote:
> It is an AcceptanceTest.

It was in revision 1, but it looks like it got changed back to 
`DistributedTest` in revision 2.


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62163/#review185086
---


On Sept. 9, 2017, 6:21 a.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62163/
> ---
> 
> (Updated Sept. 9, 2017, 6:21 a.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick 
> Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * Use HttpOperationInvoker to replace RestHttpOperationInvoker and 
> SimpleHttpOperationInvoker
> * Use one single ShellCommandController to replace all command controllers
> * do not allow execution of commands that require client side file data 
> gathering to be executed only on the locator/server
> * deprecate CommandService and CommandStatement
> * simplify CommandRequest, delete geode's ClientHttpRequest
> * fix tests
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshExecution.java
>  23f2a73acf2cf92a8b1c0c2ea2afd10392265628 
>   geode-core/build.gradle 8145a634ae706d90026ee0154bdb2eab39e956d0 
>   geode-core/src/main/java/org/apache/geode/internal/lang/Initializer.java 
> 20373710390f7496831507684504804c81cff4ee 
>   
> geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java 
> df82dffb8e6655785e5347d99032a64cc4d3b40e 
>   geode-core/src/main/java/org/apache/geode/management/MemberMXBean.java 
> ca7c2a24f1e78ab2cb3047f06f553b117fc8ba8e 
>   geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java 
> 226086f7c601292bef307313775ae26b97ce65a5 
>   
> geode-core/src/main/java/org/apache/geode/management/cli/CommandService.java 
> 20f1c75e06dd7e9fa533182274c45db230170da9 
>   
> geode-core/src/main/java/org/apache/geode/management/cli/CommandStatement.java
>  a01f08c2f09b9c762bbd4ef561ce0ba26d22dd73 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBean.java
>  271dce150fb3a93803806700cee7053f9422a8f2 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
>  5105c3d4bd8b2cfd3a2daf0e0f8208115591c8f1 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
>  3c8f6cf235d0f1b34d948d23e39fddfbe306be2c 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandRequest.java
>  00a05872a512f294f914dbc8ac1c12b799a9145d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponse.java
>  81c49583b759ea815f6f20fb1c6da7edb7f99b2f 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java
>  790e54be47673f67060d41b323f4b6d22800a852 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java
>  b228b281fb471f699c642045d9603ea9d8f9bfcc 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
>  a75eeb0ce591a9e3e1c4f56626a1cae8fe722806 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommand.java
>  4f465399abcbcf1d508e05c7fbd73bdd3c68cf1d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommand.java
>  672ec881d04d7aa47e01d58268d3df90a285d95a 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportImportClusterConfigurationCommands.java
>  83eddeebb472758944863cde098746c7ff8da5a4 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java
>  70e1e60ff6ed137900eae5d19d67497a5fa718e2 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshCommand.java
>  c7f53b1552b45f340b244828cb76d09c8aaa83da 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java
>  3c039b3a70cf7b38b4f4af77079f0b5d6a39caf

Re: Review Request 62163: GEODE-3096: pulling in refactoring work on HttpOperationInvoker

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62163/#review185108
---


Ship it!




Ship It!

- Jared Stewart


On Sept. 9, 2017, 6:21 a.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62163/
> ---
> 
> (Updated Sept. 9, 2017, 6:21 a.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick 
> Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * Use HttpOperationInvoker to replace RestHttpOperationInvoker and 
> SimpleHttpOperationInvoker
> * Use one single ShellCommandController to replace all command controllers
> * do not allow execution of commands that require client side file data 
> gathering to be executed only on the locator/server
> * deprecate CommandService and CommandStatement
> * simplify CommandRequest, delete geode's ClientHttpRequest
> * fix tests
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshExecution.java
>  23f2a73acf2cf92a8b1c0c2ea2afd10392265628 
>   geode-core/build.gradle 8145a634ae706d90026ee0154bdb2eab39e956d0 
>   geode-core/src/main/java/org/apache/geode/internal/lang/Initializer.java 
> 20373710390f7496831507684504804c81cff4ee 
>   
> geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java 
> df82dffb8e6655785e5347d99032a64cc4d3b40e 
>   geode-core/src/main/java/org/apache/geode/management/MemberMXBean.java 
> ca7c2a24f1e78ab2cb3047f06f553b117fc8ba8e 
>   geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java 
> 226086f7c601292bef307313775ae26b97ce65a5 
>   
> geode-core/src/main/java/org/apache/geode/management/cli/CommandService.java 
> 20f1c75e06dd7e9fa533182274c45db230170da9 
>   
> geode-core/src/main/java/org/apache/geode/management/cli/CommandStatement.java
>  a01f08c2f09b9c762bbd4ef561ce0ba26d22dd73 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBean.java
>  271dce150fb3a93803806700cee7053f9422a8f2 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
>  5105c3d4bd8b2cfd3a2daf0e0f8208115591c8f1 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
>  3c8f6cf235d0f1b34d948d23e39fddfbe306be2c 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandRequest.java
>  00a05872a512f294f914dbc8ac1c12b799a9145d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponse.java
>  81c49583b759ea815f6f20fb1c6da7edb7f99b2f 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java
>  790e54be47673f67060d41b323f4b6d22800a852 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java
>  b228b281fb471f699c642045d9603ea9d8f9bfcc 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
>  a75eeb0ce591a9e3e1c4f56626a1cae8fe722806 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommand.java
>  4f465399abcbcf1d508e05c7fbd73bdd3c68cf1d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommand.java
>  672ec881d04d7aa47e01d58268d3df90a285d95a 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportImportClusterConfigurationCommands.java
>  83eddeebb472758944863cde098746c7ff8da5a4 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java
>  70e1e60ff6ed137900eae5d19d67497a5fa718e2 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshCommand.java
>  c7f53b1552b45f340b244828cb76d09c8aaa83da 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java
>  3c039b3a70cf7b38b4f4af77079f0b5d6a39caf5 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandExecutor.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java
>  2464b0065d61b1aafc3b933f5f1a04e90e95c689 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandStatementImpl.java
>  ac510d1755c7dba1c2c7a772887a5c1b64cdcf57 
>   
> geode-core/src/main/java/org/apache/

Re: Review Request 62163: GEODE-3096: pulling in refactoring work on HttpOperationInvoker

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62163/#review185086
---




geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/GfshStartLocatorLogTest.java
Line 36 (original), 33 (patched)
<https://reviews.apache.org/r/62163/#comment261303>

I think this probably ought to be `AcceptanceTest`.


- Jared Stewart


On Sept. 9, 2017, 6:21 a.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62163/
> ---
> 
> (Updated Sept. 9, 2017, 6:21 a.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick 
> Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * Use HttpOperationInvoker to replace RestHttpOperationInvoker and 
> SimpleHttpOperationInvoker
> * Use one single ShellCommandController to replace all command controllers
> * do not allow execution of commands that require client side file data 
> gathering to be executed only on the locator/server
> * deprecate CommandService and CommandStatement
> * simplify CommandRequest, delete geode's ClientHttpRequest
> * fix tests
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshExecution.java
>  23f2a73acf2cf92a8b1c0c2ea2afd10392265628 
>   geode-core/build.gradle 8145a634ae706d90026ee0154bdb2eab39e956d0 
>   geode-core/src/main/java/org/apache/geode/internal/lang/Initializer.java 
> 20373710390f7496831507684504804c81cff4ee 
>   
> geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java 
> df82dffb8e6655785e5347d99032a64cc4d3b40e 
>   geode-core/src/main/java/org/apache/geode/management/MemberMXBean.java 
> ca7c2a24f1e78ab2cb3047f06f553b117fc8ba8e 
>   geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java 
> 226086f7c601292bef307313775ae26b97ce65a5 
>   
> geode-core/src/main/java/org/apache/geode/management/cli/CommandService.java 
> 20f1c75e06dd7e9fa533182274c45db230170da9 
>   
> geode-core/src/main/java/org/apache/geode/management/cli/CommandStatement.java
>  a01f08c2f09b9c762bbd4ef561ce0ba26d22dd73 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBean.java
>  271dce150fb3a93803806700cee7053f9422a8f2 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
>  5105c3d4bd8b2cfd3a2daf0e0f8208115591c8f1 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
>  3c8f6cf235d0f1b34d948d23e39fddfbe306be2c 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandRequest.java
>  00a05872a512f294f914dbc8ac1c12b799a9145d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponse.java
>  81c49583b759ea815f6f20fb1c6da7edb7f99b2f 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java
>  790e54be47673f67060d41b323f4b6d22800a852 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java
>  b228b281fb471f699c642045d9603ea9d8f9bfcc 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
>  a75eeb0ce591a9e3e1c4f56626a1cae8fe722806 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommand.java
>  4f465399abcbcf1d508e05c7fbd73bdd3c68cf1d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommand.java
>  672ec881d04d7aa47e01d58268d3df90a285d95a 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportImportClusterConfigurationCommands.java
>  83eddeebb472758944863cde098746c7ff8da5a4 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java
>  70e1e60ff6ed137900eae5d19d67497a5fa718e2 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshCommand.java
>  c7f53b1552b45f340b244828cb76d09c8aaa83da 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java
>  3c039b3a70cf7b38b4f4af77079f0b5d6a39caf5 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandExecutor.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java
>  2464b0065d61b1aafc3b933f5f1a04e90e95c68

Re: Review Request 62189: GEODE-2817: consolidate authorize(*) methods

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62189/#review185024
---


Ship it!




Ship It!

- Jared Stewart


On Sept. 8, 2017, 4:28 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62189/
> ---
> 
> (Updated Sept. 8, 2017, 4:28 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick 
> Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2817: consolidate authorize(*) methods
> 
> * this is just a changeset that removed those authorizeCluster*, 
> authorizeData* authorizeRegion* functions. They made the interface 
> unnecessaily large, and doesn't really offer much value. I prefer to see 
> authorize(DATA, READ, regionname), insteadof authorizeRegionRead(*) in the 
> code so that it's clear what exactly is the permssion needed. This also makes 
> the further change/update to the interface easier to manage.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/cache/execute/Function.java 
> 25ba4e37f579e1f222439ef19c223111faabda61 
>   
> geode-core/src/main/java/org/apache/geode/cache/execute/FunctionContext.java 
> 0b4e7f9de38b234742a427b55ceda2e63d93d83e 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/auth/GMSAuthenticator.java
>  f895b964794f99127f1f0c9564f3f85213e0af22 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/BaseCommandQuery.java
>  184aa36fc5509285001155e20d846cc717544d2f 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ClearRegion.java
>  610af436cd96b0663d69915d8a1b37549e4f7bc2 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ContainsKey.java
>  d7a1b2b0183405142d524c1d91433eb01ac3e9f6 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ContainsKey66.java
>  03e798c579e6c26e3aa7cb9a522f8af514f60ce3 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/CreateRegion.java
>  2be4724bc2b2b6d472558cdecddba982da032a08 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Destroy.java
>  cdbab8047558234bd25aa6155a7603a62d697d03 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Destroy65.java
>  c8b794a9b0b4c223e6391a8e7f63aa0747943417 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/DestroyRegion.java
>  baa2f3f480ce4bea522ea520e86328384cfd2d23 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteFunction.java
>  08f02642e937abf3b0e2557cd06c8264ac0dee32 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteFunction65.java
>  53db561963d7b9abbe86e0dbf7e73613f6a5ceef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteFunction66.java
>  a3b061f761a9f1e6a7302b5e8dc31e9a98a9959c 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteRegionFunction.java
>  73eff40c34f6fe882ec1e21168e522ae9ced2736 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteRegionFunction65.java
>  47684aa2fb5d8ec0190f0e047a627c08e77598f1 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteRegionFunction66.java
>  26d5d3f31a6b0cf2e2ff1477a2e5e3449fd5453d 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteRegionFunctionSingleHop.java
>  8c3bb381fe6e2e9018aaf8c333a3ab0c26127c27 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Get70.java
>  62644eb109f661a00c2c4df06e96c8d7a4f8d33f 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetAll.java
>  e214ce16e4be1f044f40f44308435eec43d1c8ca 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetAll651.java
>  aacfc6dccd6385ac39a9c2b65ecf6f7456bba704 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetAll70.java
>  0449447e3b9e8bd202ed7d14f35d258034f4c861 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetAllWithCallback.java
>  9f970a59961c8d10c498f8db6f8cb403036d9f96 
>   
> geode-core/src/main/jav

Re: Review Request 62132: GEODE-3277: Fix error path constructors of Launcher inner State classess

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62132/#review185020
---


Ship it!




Ship It!

- Jared Stewart


On Sept. 8, 2017, 7:58 p.m., Ken Howe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62132/
> ---
> 
> (Updated Sept. 8, 2017, 7:58 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, 
> and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Updated tests for changes in the error constructors for ServerState and
> LocatorState.
> 
> Minor spelling corrections.
> 
> This reintroduces changes that were reverted due to merge conflicts with
> the previous state of the develop branch
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 
> 83c1ab533e3dea323a8a99f7002b9464a54dfc25 
>   geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 
> ae64691605130c9b212a3a33bb65ae37b28af02b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java
>  72ccfbbc83b18e8bc32759dbeabaf2f9ef4c2f45 
>   
> geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java
>  409a96dbe416a6f96c2389356b9d823d1adb793f 
>   
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java
>  9fce94e89a369094a2383eb9103f2f43a8ff3013 
>   
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java
>  cc42a53772f3064b800ca1ac1ae894be6c715399 
>   
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java
>  29ddaaf6692565a9afb8c528790b35798d118a31 
>   
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java
>  733a1082ae9993fbdb646712380af7dcc1cca560 
>   
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java 
> 2bcd994d4d14888adfdf68abef5acbc068b6fea8 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
>  a9ce889006800523505dace6e0b4696c9911d205 
> 
> 
> Diff: https://reviews.apache.org/r/62132/diff/3/
> 
> 
> Testing
> ---
> 
> Precheckin was green except for known problem with a new protobuf test and 1 
> apparently flaky test in  HARQueueNewImplDUnitTest that passed on a second 
> run.
> 
> 
> Thanks,
> 
> Ken Howe
> 
>

Re: Review Request 62179: Test Rule Fix: clean up client DS when using LocatorServerStartupRule

[Jared Stewart]

> On Sept. 8, 2017, 4:37 p.m., Jared Stewart wrote:
> > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
> > Lines 232 (patched)
> > <https://reviews.apache.org/r/62179/diff/1-2/?file=1818168#file1818168line243>
> >
> > Do you think there is any chance someone will start both a server and a 
> > client in the same VM?  (To deal with that I think we would just remove the 
> > `else{` and always try to disconnect the DS, even if `member` was not 
> > `null`)
> 
> Jinmei Liao wrote:
> the member.stopMember() eventually calls MemberStarerRule.stopMember() 
> which will call disconnectDS already. Is it still necessary to this?

Hm, maybe I'm missing something.  All I see in MemberStarterRule.stopMember() 
is `  abstract void stopMember();`, and it doesn't look like either of the 
concrete implementations (in LocatorStarterRule and ServerStarterRule) 
explicitly disconnect the DS.  Perhaps `InternalLocator.stop()` and 
`CacheServer.stop()` already disconnect the DS?  If that's the case I don't 
think it should be necessary for us to disconnect it again.


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62179/#review184984
---


On Sept. 8, 2017, 3:51 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62179/
> -------
> 
> (Updated Sept. 8, 2017, 3:51 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick 
> Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Test Rule Fix: clean up clientVM if using LocatorServerStartUpRule to get the 
> ClientVM
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/IndexCommandsShareConfigurationDUnitTest.java
>  1a5fc3485e159ca311247e617d5bec2b37c6ee10 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowMissingDiskStoresDUnitTest.java
>  3e9227e99ad57624b024498d0ff5e807c8853221 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ImportClusterConfigDistributedTest.java
>  31c58177d60c9cf9ad0d07fc7a7daf8b332d3c9e 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
>  f0385e21f708d9a085e7129d82fb3101e2fb8322 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/MemberStarterRule.java
>  a832a2590527100afc05fb9de2e332a263d52c19 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/standalone/DUnitLauncher.java
>  b35270e2d97930cee68d8c54221a04c20dfb96de 
>   
> geode-wan/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigurationIndexWithFromClauseDUnitTest.java
>  c301d587c04651a03170e3da6451ebf2acf063c0 
> 
> 
> Diff: https://reviews.apache.org/r/62179/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 62179: Test Rule Fix: clean up client DS when using LocatorServerStartupRule

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62179/#review184984
---




geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
Line 107 (original), 107 (patched)
<https://reviews.apache.org/r/62179/#comment261210>

I like this use of `IntStream`!



geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
Lines 232 (patched)
<https://reviews.apache.org/r/62179/#comment261209>

Do you think there is any chance someone will start both a server and a 
client in the same VM?  (To deal with that I think we would just remove the 
`else{` and always try to disconnect the DS, even if `member` was not `null`)


- Jared Stewart


On Sept. 8, 2017, 3:51 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62179/
> ---
> 
> (Updated Sept. 8, 2017, 3:51 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick 
> Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Test Rule Fix: clean up clientVM if using LocatorServerStartUpRule to get the 
> ClientVM
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/IndexCommandsShareConfigurationDUnitTest.java
>  1a5fc3485e159ca311247e617d5bec2b37c6ee10 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowMissingDiskStoresDUnitTest.java
>  3e9227e99ad57624b024498d0ff5e807c8853221 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ImportClusterConfigDistributedTest.java
>  31c58177d60c9cf9ad0d07fc7a7daf8b332d3c9e 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
>  f0385e21f708d9a085e7129d82fb3101e2fb8322 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/MemberStarterRule.java
>  a832a2590527100afc05fb9de2e332a263d52c19 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/standalone/DUnitLauncher.java
>  b35270e2d97930cee68d8c54221a04c20dfb96de 
>   
> geode-wan/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigurationIndexWithFromClauseDUnitTest.java
>  c301d587c04651a03170e3da6451ebf2acf063c0 
> 
> 
> Diff: https://reviews.apache.org/r/62179/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 62179: Test Rule Fix: clean up client DS when using LocatorServerStartupRule

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62179/#review184915
---




geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
Lines 107 (patched)
<https://reviews.apache.org/r/62179/#comment261114>

How do you think this reads this instead of the `for` loop? 
```
Consumer stopMemberAndDisconnectDS = (MemberVM memberVM) -> {
  memberVM.stopMember();
  memberVM.getVM().invoke((SerializableRunnableIF) 
MemberStarterRule::disconnectDSIfAny);
};


Arrays.stream(members).filter(Objects::nonNull).forEach(stopMemberAndDisconnectDS);
    ```


- Jared Stewart


On Sept. 7, 2017, 11:12 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62179/
> ---
> 
> (Updated Sept. 7, 2017, 11:12 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick 
> Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Test Rule Fix: clean up clientVM if using LocatorServerStartUpRule to get the 
> ClientVM
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
>  f0385e21f708d9a085e7129d82fb3101e2fb8322 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/MemberStarterRule.java
>  a832a2590527100afc05fb9de2e332a263d52c19 
> 
> 
> Diff: https://reviews.apache.org/r/62179/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 62163: GEODE-3096: pulling in refactoring work on HttpOperationInvoker

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62163/#review184901
---




geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshExecution.java
Lines 37 (patched)
<https://reviews.apache.org/r/62163/#comment261098>

I don't think this method is used.



geode-core/src/main/java/org/apache/geode/management/cli/CommandService.java
Line 109 (original)
<https://reviews.apache.org/r/62163/#comment261101>

I wonder if these methods are OK to delete, since they're public methods in 
what looks to be a public (non-internal) class.



geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java
Line 86 (original)
<https://reviews.apache.org/r/62163/#comment261107>

Why is it safe to remove this escaping now?



geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
Line 256 (original), 252 (patched)
<https://reviews.apache.org/r/62163/#comment261103>

It might be good to add a comment here explaining that our intent is to 
trigger authorization.


- Jared Stewart


On Sept. 7, 2017, 3:33 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62163/
> ---
> 
> (Updated Sept. 7, 2017, 3:33 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick 
> Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * Use HttpOperationInvoker to replace RestHttpOperationInvoker and 
> SimpleHttpOperationInvoker
> * Use one single ShellCommandController to replace all command controllers
> * do not allow execution of commands that require client side file data 
> gathering to be executed only on the locator/server
> * deprecate CommandService and CommandStatement
> * simplify CommandRequest, delete geode's ClientHttpRequest
> * fix tests
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/GfshStartLocatorLogTest.java
>  eadbea8a2ddde34aed7e1a39e4826be4c70fd65f 
>   
> geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshExecution.java
>  23f2a73acf2cf92a8b1c0c2ea2afd10392265628 
>   geode-core/build.gradle 8145a634ae706d90026ee0154bdb2eab39e956d0 
>   geode-core/src/main/java/org/apache/geode/internal/lang/Initializer.java 
> 20373710390f7496831507684504804c81cff4ee 
>   
> geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java 
> df82dffb8e6655785e5347d99032a64cc4d3b40e 
>   geode-core/src/main/java/org/apache/geode/management/MemberMXBean.java 
> ca7c2a24f1e78ab2cb3047f06f553b117fc8ba8e 
>   geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java 
> 226086f7c601292bef307313775ae26b97ce65a5 
>   
> geode-core/src/main/java/org/apache/geode/management/cli/CommandService.java 
> 20f1c75e06dd7e9fa533182274c45db230170da9 
>   
> geode-core/src/main/java/org/apache/geode/management/cli/CommandStatement.java
>  a01f08c2f09b9c762bbd4ef561ce0ba26d22dd73 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBean.java
>  271dce150fb3a93803806700cee7053f9422a8f2 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
>  5105c3d4bd8b2cfd3a2daf0e0f8208115591c8f1 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
>  3c8f6cf235d0f1b34d948d23e39fddfbe306be2c 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandRequest.java
>  00a05872a512f294f914dbc8ac1c12b799a9145d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponse.java
>  81c49583b759ea815f6f20fb1c6da7edb7f99b2f 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java
>  790e54be47673f67060d41b323f4b6d22800a852 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java
>  b228b281fb471f699c642045d9603ea9d8f9bfcc 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
>  a75eeb0ce591a9e3e1c4f56626a1cae8fe722806 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommand.java
>  4f465399abcbcf1d508e05c7fbd73bdd3c68cf1d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommand.java
>  672ec881d04d7aa47e01d58268d3df90a285d95a 
&

Review Request 62178: GEODE-3449: Fix flakiness in ConnectCommandWithSSLTest

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62178/
---

Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3449: Fix flakiness in ConnectCommandWithSSLTest


Diffs
-

  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/IndexCommandsDUnitTest.java
 794c58846ffe9c1b2b5143c95b9dc5aea388afbc 


Diff: https://reviews.apache.org/r/62178/diff/1/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 62132: GEODE-3277: Fix error path constructors of Launcher inner State classess

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62132/#review184852
---




geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
Line 1099 (original), 1092 (patched)
<https://reviews.apache.org/r/62132/#comment261049>

Do you think there is any value in logging the full stacktrace of the 
exception?  It looks like that never gets logged anywhere.


- Jared Stewart


On Sept. 6, 2017, 8:10 p.m., Ken Howe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62132/
> ---
> 
> (Updated Sept. 6, 2017, 8:10 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick 
> Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Updated tests for changes in the error constructors for ServerState and
> LocatorState.
> 
> Minor spelling corrections.
> 
> This reintroduces changes that were reverted due to merge conflicts with
> the previous state of the develop branch
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 
> 83c1ab533e3dea323a8a99f7002b9464a54dfc25 
>   geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 
> ae64691605130c9b212a3a33bb65ae37b28af02b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java
>  72ccfbbc83b18e8bc32759dbeabaf2f9ef4c2f45 
>   
> geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java
>  409a96dbe416a6f96c2389356b9d823d1adb793f 
>   
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java
>  9fce94e89a369094a2383eb9103f2f43a8ff3013 
>   
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java
>  cc42a53772f3064b800ca1ac1ae894be6c715399 
>   
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java
>  29ddaaf6692565a9afb8c528790b35798d118a31 
>   
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java
>  733a1082ae9993fbdb646712380af7dcc1cca560 
>   
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java 
> 2bcd994d4d14888adfdf68abef5acbc068b6fea8 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
>  a9ce889006800523505dace6e0b4696c9911d205 
> 
> 
> Diff: https://reviews.apache.org/r/62132/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin is green
> 
> 
> Thanks,
> 
> Ken Howe
> 
>

Re: Missing Gitbox activation email

[Jared Stewart]

To anyone else having the same problem, I was able to accept the invite by 
navigating to https://github.com/apache <https://github.com/apache>.  

 - Jared
> On Sep 7, 2017, at 9:36 AM, Udo Kohlmeyer <ukohlme...@pivotal.io> wrote:
> 
> It was as Dan indicated you need to make sure your github username is 
> registered with id.apache.org. I then later received an email that requested 
> I join the apache org. And now my gitbox works.
> 
> Thank you for the help.
> 
> --Udo
> 
> 
> On 9/6/17 21:41, Jacob Barrett wrote:
>> I think the key is exactly what Dan pointed out about accepting the Apache 
>> org invite on GitHub. If you hadn’t done that previously you must do it now. 
>> You must have any and all GitHub IDs you plan to use as committer registered 
>> with Apache. You should also have any and all email addresses you plan to 
>> use as committer registered as well.
>> 
>> -Jake
>> 
>> 
>>> On Sep 6, 2017, at 8:07 PM, Ernest Burghardt <eburgha...@pivotal.io> wrote:
>>> 
>>> When geode-native moved I don't think we received an activation email...
>>> just had to follow the instructions Jake sent out: (obviously drop the
>>> "-native")
>>> 
>>> For committers the origin is now github.com/apache/geode-native. Before you
>>> can get write access to the new repo you need to "link" your accounts via
>>> https://gitbox.apache.org/setup/. You should also change the URL of your
>>> origin from Apache to GitHub. You will also notice we have full control
>>> over pull requests, including assignments and labeling. Good times ahead.
>>> 
>>>> On Wed, Sep 6, 2017 at 5:53 PM, Xiaojian Zhou <gz...@pivotal.io> wrote:
>>>> 
>>>> I did not get any email. But it seems all set for me.
>>>> 
>>>> ​
>>>> 
>>>>> On Wed, Sep 6, 2017 at 4:33 PM, Nabarun Nag <n...@apache.org> wrote:
>>>>> 
>>>>> *I think the email takes some time to arrive."An organisational invite
>>>>> will
>>>>> be sent to you via email shortly thereafter (within 30 minutes)."*
>>>>> 
>>>>>> On Wed, Sep 6, 2017 at 4:21 PM Dan Smith <dsm...@pivotal.io> wrote:
>>>>>> 
>>>>>> If you are stuck on 3rd step (MFA Status) make you have added your
>>>>> github
>>>>>> username on id.apache.org *and* that you have accepted the invitation
>>>>> to
>>>>>> join the apache group on github.
>>>>>> 
>>>>>> You should see an apache feather listed underneath your organizations on
>>>>>> github.
>>>>>> 
>>>>>> -Dan
>>>>>> 
>>>>>> On Wed, Sep 6, 2017 at 4:08 PM, Jacob Barrett <jbarr...@pivotal.io>
>>>>> wrote:
>>>>>>> I’d hit up Infra tomorrow if it isn’t working by then.
>>>>>>> 
>>>>>>>> On Sep 6, 2017, at 4:06 PM, Jared Stewart <jstew...@pivotal.io>
>>>>> wrote:
>>>>>>>> I’m stuck on the same step.  I tried clearing out my GitHub
>>>>> username at
>>>>>>> id.apache.org <http://id.apache.org/> and then re-adding it in the
>>>>> hopes
>>>>>>> of re-triggering the email, but it still hasn’t arrived.
>>>>>>>> - Jared
>>>>>>>>> On Sep 6, 2017, at 4:04 PM, Udo Kohlmeyer <u...@apache.org> wrote:
>>>>>>>>> 
>>>>>>>>> Hey there,
>>>>>>>>> 
>>>>>>>>> I've gone through all the steps to activate my github user with
>>>>>> gitbox.
>>>>>>>>> Looking at gitbox.apache.org, I was completed step 2 of 3. I'm
>>>>>>> currently waiting for the email that I'm supposed to receive, BUT it
>>>>> has
>>>>>>> never arrived. I have checked my Spam folder and it was not in there
>>>>>> either.
>>>>>>>>> Does anyone know how to have to email sent *again*?
>>>>>>>>> 
>>>>>>>>> --Udo
>>>> 
> 

Re: Missing Gitbox activation email

[Jared Stewart]

I never received an Apache org invite on GitHub or via email after adding my 
GitHub username to id.apache.org.  I filed an infra ticket to request help.  
(https://issues.apache.org/jira/browse/INFRA-15042 
<https://issues.apache.org/jira/browse/INFRA-15042>)

- Jared
> On Sep 7, 2017, at 9:36 AM, Udo Kohlmeyer <ukohlme...@pivotal.io> wrote:
> 
> It was as Dan indicated you need to make sure your github username is 
> registered with id.apache.org. I then later received an email that requested 
> I join the apache org. And now my gitbox works.
> 
> Thank you for the help.
> 
> --Udo
> 
> 
> On 9/6/17 21:41, Jacob Barrett wrote:
>> I think the key is exactly what Dan pointed out about accepting the Apache 
>> org invite on GitHub. If you hadn’t done that previously you must do it now. 
>> You must have any and all GitHub IDs you plan to use as committer registered 
>> with Apache. You should also have any and all email addresses you plan to 
>> use as committer registered as well.
>> 
>> -Jake
>> 
>> 
>>> On Sep 6, 2017, at 8:07 PM, Ernest Burghardt <eburgha...@pivotal.io> wrote:
>>> 
>>> When geode-native moved I don't think we received an activation email...
>>> just had to follow the instructions Jake sent out: (obviously drop the
>>> "-native")
>>> 
>>> For committers the origin is now github.com/apache/geode-native. Before you
>>> can get write access to the new repo you need to "link" your accounts via
>>> https://gitbox.apache.org/setup/. You should also change the URL of your
>>> origin from Apache to GitHub. You will also notice we have full control
>>> over pull requests, including assignments and labeling. Good times ahead.
>>> 
>>>> On Wed, Sep 6, 2017 at 5:53 PM, Xiaojian Zhou <gz...@pivotal.io> wrote:
>>>> 
>>>> I did not get any email. But it seems all set for me.
>>>> 
>>>> ​
>>>> 
>>>>> On Wed, Sep 6, 2017 at 4:33 PM, Nabarun Nag <n...@apache.org> wrote:
>>>>> 
>>>>> *I think the email takes some time to arrive."An organisational invite
>>>>> will
>>>>> be sent to you via email shortly thereafter (within 30 minutes)."*
>>>>> 
>>>>>> On Wed, Sep 6, 2017 at 4:21 PM Dan Smith <dsm...@pivotal.io> wrote:
>>>>>> 
>>>>>> If you are stuck on 3rd step (MFA Status) make you have added your
>>>>> github
>>>>>> username on id.apache.org *and* that you have accepted the invitation
>>>>> to
>>>>>> join the apache group on github.
>>>>>> 
>>>>>> You should see an apache feather listed underneath your organizations on
>>>>>> github.
>>>>>> 
>>>>>> -Dan
>>>>>> 
>>>>>> On Wed, Sep 6, 2017 at 4:08 PM, Jacob Barrett <jbarr...@pivotal.io>
>>>>> wrote:
>>>>>>> I’d hit up Infra tomorrow if it isn’t working by then.
>>>>>>> 
>>>>>>>> On Sep 6, 2017, at 4:06 PM, Jared Stewart <jstew...@pivotal.io>
>>>>> wrote:
>>>>>>>> I’m stuck on the same step.  I tried clearing out my GitHub
>>>>> username at
>>>>>>> id.apache.org <http://id.apache.org/> and then re-adding it in the
>>>>> hopes
>>>>>>> of re-triggering the email, but it still hasn’t arrived.
>>>>>>>> - Jared
>>>>>>>>> On Sep 6, 2017, at 4:04 PM, Udo Kohlmeyer <u...@apache.org> wrote:
>>>>>>>>> 
>>>>>>>>> Hey there,
>>>>>>>>> 
>>>>>>>>> I've gone through all the steps to activate my github user with
>>>>>> gitbox.
>>>>>>>>> Looking at gitbox.apache.org, I was completed step 2 of 3. I'm
>>>>>>> currently waiting for the email that I'm supposed to receive, BUT it
>>>>> has
>>>>>>> never arrived. I have checked my Spam folder and it was not in there
>>>>>> either.
>>>>>>>>> Does anyone know how to have to email sent *again*?
>>>>>>>>> 
>>>>>>>>> --Udo
>>>> 
> 

Re: Missing Gitbox activation email

[Jared Stewart]

I’m stuck on the same step.  I tried clearing out my GitHub username at 
id.apache.org  and then re-adding it in the hopes of 
re-triggering the email, but it still hasn’t arrived.

- Jared
> On Sep 6, 2017, at 4:04 PM, Udo Kohlmeyer  wrote:
> 
> Hey there,
> 
> I've gone through all the steps to activate my github user with gitbox.
> 
> Looking at gitbox.apache.org, I was completed step 2 of 3. I'm currently 
> waiting for the email that I'm supposed to receive, BUT it has never arrived. 
> I have checked my Spam folder and it was not in there either.
> 
> Does anyone know how to have to email sent *again*?
> 
> --Udo

Re: Review Request 62022: GEODE-3549: fix the constantly failing flaky tests

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62022/#review184320
---


Ship it!




Ship It!

- Jared Stewart


On Aug. 31, 2017, 9:22 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62022/
> ---
> 
> (Updated Aug. 31, 2017, 9:22 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3549: fix the constantly failing flaky tests
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java
>  e91e3657e28bdbba2016eb5b6a84dfa434af85e9 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExecuteFunctionCommand.java
>  46ad6e5e2eb31cc2cfdb8b48c7d514efceb865ae 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java
>  8a16aa4a76cc98d4911b90b79527970bfaaf3510 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/FunctionCommandsController.java
>  da3fc65db0b5098890237ac4bdcd35b15b929398 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java
>  961cdf87f883957532a1d1abded8320df96edb3d 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java
>  0247c79badef9d9929f3dd5ba1cc88d90d23c7c7 
> 
> 
> Diff: https://reviews.apache.org/r/62022/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Review Request 62021: GEODE-3547: Simplify behavior for non-writable deploy directory

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62021/
---

Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3547: Simplify behavior for non-writable deploy directory
 - Also fixes the flaky JarDeployerIntegrationTest


Diffs
-

  geode-core/src/main/java/org/apache/geode/internal/JarDeployer.java 
c21fb9dd1ad1ff5f64a6bee447905c8636d93c83 
  
geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java
 b81e3e9d095b03fc24287808e9df06c46900fa81 


Diff: https://reviews.apache.org/r/62021/diff/1/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 62002: GEODE-3539: Add tests for List Members and Describe Member

[Jared Stewart]

> On Aug. 31, 2017, 3:25 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java
> > Lines 632 (patched)
> > <https://reviews.apache.org/r/62002/diff/1/?file=1808175#file1808175line632>
> >
> > See Gfsh.handleExecutionResult
> > 
> > any chance this could be called before gfsh gets the result for 
> > display? If so, then gfsh will display nothing, since hasNextLine() will 
> > return false from now on. Maybe for now just add a comment there saying 
> > only call this after gfsh displays the result already. The whole gfsh 
> > result displayer is a mess for now.
> 
> Jared Stewart wrote:
> I only intended to use this method from tests and I think in that case 
> there should always be a result.  I thought about adding it to some sort of 
> helper/utility class (currently it resides in CliCommandTestBase), but it 
> seems like it makes sense for it to live in CommandResult.  I'll add a 
> comment with some explanation of how it should be used.
> 
> Jinmei Liao wrote:
> Currently, without this function, I get the command result string using 
> gfshRule.getoutputString method

Oh, I hadn't noticed that method.  I will use that for now instead.


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62002/#review184275
---


On Aug. 30, 2017, 10:13 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62002/
> -------
> 
> (Updated Aug. 30, 2017, 10:13 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, 
> and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3539: Add tests for List Members and Describe Member
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListMemberCommand.java
>  ea88c69ebdd2ce5ffbab486fcb7a4dda71935586 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java
>  bbb59d0755ffd2cf405f78c89b420a5279844e29 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java
>  ba5c788f90ef68dc8ac338a4619646b4f3608293 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DescribeMembersCommandDUnitTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListMembersCommandDUnitTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java
>  fe6bc404d33b48e5384348241c17ccf924f4627c 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java
>  17719be16338ab9894878660054b75ff9cc6c3ec 
> 
> 
> Diff: https://reviews.apache.org/r/62002/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 62002: GEODE-3539: Add tests for List Members and Describe Member

[Jared Stewart]

> On Aug. 31, 2017, 3:25 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java
> > Lines 632 (patched)
> > <https://reviews.apache.org/r/62002/diff/1/?file=1808175#file1808175line632>
> >
> > See Gfsh.handleExecutionResult
> > 
> > any chance this could be called before gfsh gets the result for 
> > display? If so, then gfsh will display nothing, since hasNextLine() will 
> > return false from now on. Maybe for now just add a comment there saying 
> > only call this after gfsh displays the result already. The whole gfsh 
> > result displayer is a mess for now.

I only intended to use this method from tests and I think in that case there 
should always be a result.  I thought about adding it to some sort of 
helper/utility class (currently it resides in CliCommandTestBase), but it seems 
like it makes sense for it to live in CommandResult.  I'll add a comment with 
some explanation of how it should be used.


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62002/#review184275
-------


On Aug. 30, 2017, 10:13 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62002/
> ---
> 
> (Updated Aug. 30, 2017, 10:13 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, 
> and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3539: Add tests for List Members and Describe Member
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListMemberCommand.java
>  ea88c69ebdd2ce5ffbab486fcb7a4dda71935586 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java
>  bbb59d0755ffd2cf405f78c89b420a5279844e29 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java
>  ba5c788f90ef68dc8ac338a4619646b4f3608293 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DescribeMembersCommandDUnitTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListMembersCommandDUnitTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java
>  fe6bc404d33b48e5384348241c17ccf924f4627c 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java
>  17719be16338ab9894878660054b75ff9cc6c3ec 
> 
> 
> Diff: https://reviews.apache.org/r/62002/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Review Request 62002: GEODE-3539: Add tests for List Members and Describe Member

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62002/
---

Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3539: Add tests for List Members and Describe Member


Diffs
-

  
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListMemberCommand.java
 ea88c69ebdd2ce5ffbab486fcb7a4dda71935586 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java
 bbb59d0755ffd2cf405f78c89b420a5279844e29 
  
geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java
 ba5c788f90ef68dc8ac338a4619646b4f3608293 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DescribeMembersCommandDUnitTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListMembersCommandDUnitTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java
 fe6bc404d33b48e5384348241c17ccf924f4627c 
  
geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java
 17719be16338ab9894878660054b75ff9cc6c3ec 


Diff: https://reviews.apache.org/r/62002/diff/1/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Review Request 62001: GEODE-3525: Dockerize AcceptanceTests

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62001/
---

Review request for geode, Anthony Baker, Jens Deppe, Jinmei Liao, Jared 
Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3525: Dockerize AcceptanceTests


Diffs
-

  gradle/docker.gradle b5a356f6222848f672261cf0050c02044b3c112e 


Diff: https://reviews.apache.org/r/62001/diff/1/


Testing
---

Passed precheckin, both with and without the -PparallelDUnit flag set.


Thanks,

Jared Stewart

Re: Review Request 61995: wip

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61995/#review184174
---


Ship it!




Ship It!

- Jared Stewart


On Aug. 30, 2017, 5:38 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61995/
> ---
> 
> (Updated Aug. 30, 2017, 5:38 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3539: add test for invalid command
> 
> * also rework GfshCommandIntegrationTest in assembly module
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/GfshCommandIntegrationTest.java
>  263b12cbdee2bc5637e64fdad0e34262ab5f25e7 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GfshCommandIntegrationTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/StartMemberUtilsTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61995/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 61974: GEODE-2859: Fix race condition in ShowDeadlockDUnitTest

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61974/
---

(Updated Aug. 29, 2017, 10:46 p.m.)


Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Geode-junit needs Awaitility


Repository: geode


Description
---

GEODE-2859: Fix race condition in ShowDeadlockDUnitTest


Diffs (updated)
-

  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
 4df0b96 
  
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
 dc17d03 
  geode-junit/build.gradle 7c533ad 
  
geode-junit/src/main/java/org/apache/geode/test/concurrent/FileBasedCountDownLatch.java
 PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/concurrent/FileBasedCountDownLatchTest.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61974/diff/2/

Changes: https://reviews.apache.org/r/61974/diff/1-2/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61972: GEODE-3445: Convert connect acceptance test to DUnit test

[Jared Stewart]

> On Aug. 29, 2017, 6:42 p.m., Patrick Rhomberg wrote:
> > As an aside, I assume you post these things via script.  You're still 
> > tagging `eyeh`.  Not that it really matters, since it's hooked ot a 
> > `pivotal` email, but still worth updating.
> 
> Patrick Rhomberg wrote:
> Also looks like you're not tagging Jinmei or Swap.

Thanks, I updated my .reviewboardrc!


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61972/#review184081
---


On Aug. 29, 2017, 4:53 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61972/
> ---
> 
> (Updated Aug. 29, 2017, 4:53 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3445: Convert connect w/ http & ssl acceptance test to DUnit test
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/GfshConnectToLocatorWithSSLAcceptanceTest.java
>  75d60a32411582d75eb0f5cacce1536a6f724a26 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandWithSSLTest.java
>  7c4fb446ca4909c628010087b7c85aa121883894 
> 
> 
> Diff: https://reviews.apache.org/r/61972/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin passed
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Review Request 61974: GEODE-2859: Fix race condition in ShowDeadlockDUnitTest

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61974/
---

Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-2859: Fix race condition in ShowDeadlockDUnitTest


Diffs
-

  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
 4df0b96cb2b86e83f612b6b7d12009801c3704af 
  
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
 dc17d036cd89ab248a7f2903647f013e6de09691 
  
geode-junit/src/main/java/org/apache/geode/test/concurrent/FileBasedCountDownLatch.java
 PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/concurrent/FileBasedCountDownLatchTest.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61974/diff/1/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61972: GEODE-3445: Convert connect acceptance test to DUnit test

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61972/
---

(Updated Aug. 29, 2017, 4:53 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3445: Convert connect w/ http & ssl acceptance test to DUnit test


Diffs
-

  
geode-assembly/src/test/java/org/apache/geode/management/GfshConnectToLocatorWithSSLAcceptanceTest.java
 75d60a32411582d75eb0f5cacce1536a6f724a26 
  
geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandWithSSLTest.java
 7c4fb446ca4909c628010087b7c85aa121883894 


Diff: https://reviews.apache.org/r/61972/diff/1/


Testing (updated)
---

Precheckin passed


Thanks,

Jared Stewart

Review Request 61972: GEODE-3445: Convert connect acceptance test to DUnit test

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61972/
---

Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3445: Convert connect w/ http & ssl acceptance test to DUnit test


Diffs
-

  
geode-assembly/src/test/java/org/apache/geode/management/GfshConnectToLocatorWithSSLAcceptanceTest.java
 75d60a32411582d75eb0f5cacce1536a6f724a26 
  
geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandWithSSLTest.java
 7c4fb446ca4909c628010087b7c85aa121883894 


Diff: https://reviews.apache.org/r/61972/diff/1/


Testing
---


Thanks,

Jared Stewart

Re: Review Request 61860: GEODE-3510: GfshRule displays output from StdError

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61860/
---

(Updated Aug. 25, 2017, 9:12 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Remove getAllOutputLines() and remove an unused file.


Repository: geode


Description
---

GEODE-3510: GfshRule displays output from StdError


Diffs (updated)
-

  
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/ProcessLogger.java
 47f0304 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ExtendsAbstractFunction.java
 cf7c7a2 


Diff: https://reviews.apache.org/r/61860/diff/2/

Changes: https://reviews.apache.org/r/61860/diff/1-2/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61860: GEODE-3510: GfshRule displays output from StdError

[Jared Stewart]

> On Aug. 24, 2017, 9:29 p.m., Kirk Lund wrote:
> > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/ProcessLogger.java
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/61860/diff/1/?file=1802513#file1802513line96>
> >
> > I'm curious, what's the result of this method? Does in interleave 
> > stdout and stderr in someway?
> > 
> > Another alternative is to use ProcessBuilder.redirectErrorStream so 
> > they become interleaved chronologically. The downside is you can't 
> > differentiate between the two. But there's probably some other way to 
> > synchronize the two chronologically using a stream tee or union.

I'll remove this method for now until I have a better implementation.


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61860/#review183798
-------


On Aug. 23, 2017, 8:18 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61860/
> ---
> 
> (Updated Aug. 23, 2017, 8:18 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3510: GfshRule displays output from StdError
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/ProcessLogger.java
>  47f030471a988055400a71e5b564f3b24397c2e8 
> 
> 
> Diff: https://reviews.apache.org/r/61860/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Review Request 61860: GEODE-3510: GfshRule displays output from StdError

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61860/
---

Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3510: GfshRule displays output from StdError


Diffs
-

  
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/ProcessLogger.java
 47f030471a988055400a71e5b564f3b24397c2e8 


Diff: https://reviews.apache.org/r/61860/diff/1/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Nightly build failures caused by attempted use of default ports

[Jared Stewart]

I think we just need to have AcceptanceTest (and possibly IntegrationTest) run 
inside Docker like DistributedTest already does.

- Jared.

> On Aug 23, 2017, at 11:32 AM, Anilkumar Gingade  wrote:
> 
>>> 1) use Docker for AcceptanceTest and IntegrationTest targets?
> To be clear, the failing tests are only in Acceptance Test and Integration
> Tests? And distributed tests are not seeing this issue as they are running
> in docker nowIf moving docker address this issue, my vote is for moving
> docker; this makes all the tests to be run in similar environment.
> 
>>> 2) not test default ports?
> If the product supports default port; we need to have test for that...Most
> of the early product evaluation is done with default port...
> 
> -Anil.
> 
> 
> On Wed, Aug 23, 2017 at 11:15 AM, Kirk Lund  wrote:
> 
>> The following nightly build failures are tests that are testing default
>> ports which are failing because the port is not available.
>> 
>> Should we:
>> 
>> 1) use Docker for AcceptanceTest and IntegrationTest targets?
>> 
>> 2) not test default ports?
>> 
>> 3) use a hacky System property to force Geode to think that some other port
>> is the default port?
>> 
>> 4) some other solution?
>> 
>> testGet – org.apache.geode.rest.internal.web.RestServersJUnitTest
>> a few seconds
>> testServerStartedOnDefaultPort –
>> org.apache.geode.rest.internal.web.RestServersJUnitTest
>> a few seconds
>> offlineStatusCommandShouldSucceedWhenConnected_server_dir –
>> org.apache.geode.management.internal.cli.shell.
>> GfshExitCodeStatusCommandsTest
>> a few seconds
>> offlineStatusCommandShouldSucceedWhenConnected_server_pid –
>> org.apache.geode.management.internal.cli.shell.
>> GfshExitCodeStatusCommandsTest
>> a few seconds
>> onlineStatusCommandShouldSucceedWhenConnected_locator_host_and_port –
>> org.apache.geode.management.internal.cli.shell.
>> GfshExitCodeStatusCommandsTest
>> a few seconds
>> offlineStatusCommandShouldSucceedEvenWhenNotConnected_server_dir –
>> org.apache.geode.management.internal.cli.shell.
>> GfshExitCodeStatusCommandsTest
>> a few seconds
>> offlineStatusCommandShouldSucceedEvenWhenNotConnected_server_pid –
>> org.apache.geode.management.internal.cli.shell.
>> GfshExitCodeStatusCommandsTest
>> a few seconds
>> onlineStatusCommandShouldSucceedWhenConnected_server_name –
>> org.apache.geode.management.internal.cli.shell.
>> GfshExitCodeStatusCommandsTest
>> a few seconds
>> offlineStatusCommandShouldSucceedWhenConnected_locator_dir –
>> org.apache.geode.management.internal.cli.shell.
>> GfshExitCodeStatusCommandsTest
>> a few seconds
>> offlineStatusCommandShouldSucceedWhenConnected_locator_pid –
>> org.apache.geode.management.internal.cli.shell.
>> GfshExitCodeStatusCommandsTest
>> a few seconds
>> onlineStatusCommandShouldSucceedWhenConnected_locator_name –
>> org.apache.geode.management.internal.cli.shell.
>> GfshExitCodeStatusCommandsTest
>> a few seconds
>> onlineStatusCommandShouldSucceedWhenConnected_locator_port –
>> org.apache.geode.management.internal.cli.shell.
>> GfshExitCodeStatusCommandsTest
>> a few seconds
>> statusLocatorSucceedsWhenConnected –
>> org.apache.geode.management.internal.cli.commands.
>> StatusLocatorRealGfshTest
>> 

Re: Gitbox enables the full GitHub workflow

[Jared Stewart]

+1 for moving the other repos to Gitbox

On Aug 22, 2017 10:43 AM, "Anthony Baker"  wrote:


> On Aug 7, 2017, at 6:09 PM, Roman Shaposhnik  wrote:
>
> Hi!
>
> it has just come to my attention that Gitbox at ASF
> has been enabling full GitHub workflow (with being
> able to click Merge this PR button, etc.) for quite
> some time.
>
> This basically allows a project to have GH as a R/W
> repo as opposed to R/O mirror of what we all currnently
> have: https://gitbox.apache.org/repos/asf
>
> Personally I'm not sure I like GH workflow all that much,
> but if there's interest -- you can opt-in into Gitbox. Once
> you do -- your source of truth moves to GH. You can't
> have it both ways with git-wip-us.apache.org and Gitbox.
>
> Thanks,
> Roman.

Now that we’ve got some experience with gitbox on geode-native, are we
ready to convert the other repos?

- geode
- geode-site
- geode-examples

I think we should.

Anthony

Re: Review Request 61701: GEODE-3277: Fix error path constructors of Launcher inner State classess

[Jared Stewart]

> On Aug. 17, 2017, 5:35 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
> > Lines 2017 (patched)
> > <https://reviews.apache.org/r/61701/diff/1/?file=1798850#file1798850line2020>
> >
> > I think this double-ternary might be easier to read if we split it up 
> > into a few separate statements.
> 
> Ken Howe wrote:
> Agree with you that the double ternary sin't the most readable construct. 
> However, I ended up using it because the `this()` call has to be the first 
> statement in the constructor. The conditionals guard against NPEs from the 
> possible locations where this error constructor is called.
> 
> I'm open to other refactoring suggestions to make this easier to 
> understand.

I would suggest this sort of pattern: 

```
public LocatorState(final LocatorLauncher launcher, final Status status,
final String errorMessage) {
  this(status, // status
  errorMessage, // statusMessage
  System.currentTimeMillis(), // timestamp
  getLocatorLocation(launcher),
  null, // pid
  0L, // uptime
  launcher.getWorkingDirectory(), // workingDirectory
  ManagementFactory.getRuntimeMXBean().getInputArguments(), // 
jvmArguments
  null, // classpath
  GemFireVersion.getGemFireVersion(), // gemfireVersion
  System.getProperty("java.version"), // javaVersion
  null, // logFile
  launcher.getBindAddressAsString(), // host
  launcher.getPortAsString(), // port
  null);// memberName
}

private static String getLocatorLocation(LocatorLauncher launcher) {
  return launcher.getPort() == null ? launcher.getId()
  : HostUtils.getLocatorId((launcher.getBindAddress() == null)
  ? HostUtils.getLocalHost() : 
launcher.getBindAddress().getCanonicalHostName(),
  launcher.getPort());
}
```

where then you can split up them implementation of `getLocatorLocation` into 
several separate lines.


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61701/#review183145
---


On Aug. 16, 2017, 9:21 p.m., Ken Howe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61701/
> ---
> 
> (Updated Aug. 16, 2017, 9:21 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, 
> and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Updated tests for changes in the error constructors for ServerState and
> LocatorState.
> 
> Minor spelling corrections.
> 
> I've updated the fix for GEODE-3277 and rebased on top of the @klund's recent 
> process controller updates
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java
>  3a98373938e3de21da6badcf460dae3648218ac6 
>   geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 
> 83c1ab533e3dea323a8a99f7002b9464a54dfc25 
>   geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 
> ae64691605130c9b212a3a33bb65ae37b28af02b 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java
>  dd5841f4cffca38da07a11f381cf4174d7264349 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
>  e7f17ef208a1708f385c7c4041affb70fd309a4c 
> 
> 
> Diff: https://reviews.apache.org/r/61701/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin is in progress.
> 
> 
> Thanks,
> 
> Ken Howe
> 
>

Re: Review Request 61802: GEODE-3445: Add gfsh connect option --skip-ssl-validation

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61802/
---

(Updated Aug. 21, 2017, 10:09 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Update help text


Repository: geode


Description
---

GEODE-3445: Add gfsh connect option --skip-ssl-validation


Diffs (updated)
-

  
geode-assembly/src/test/java/org/apache/geode/management/GfshConnectToLocatorWithSSLAcceptanceTest.java
 PRE-CREATION 
  
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java
 fa25f14 
  
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshScript.java
 52ef0d3 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
 274f61c 


Diff: https://reviews.apache.org/r/61802/diff/2/

Changes: https://reviews.apache.org/r/61802/diff/1-2/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Review Request 61802: GEODE-3445: Add gfsh connect option --skip-ssl-validation

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61802/
---

Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3445: Add gfsh connect option --skip-ssl-validation


Diffs
-

  
geode-assembly/src/test/java/org/apache/geode/management/GfshConnectToLocatorWithSSLAcceptanceTest.java
 PRE-CREATION 
  
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java
 fa25f14328d2cc56b2e0b64d834ed88dde082e8f 
  
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshScript.java
 52ef0d35a176c6522664eb18449d1c4b635f16a6 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
 274f61c1f304576f8d8db1d5289875ecea706962 


Diff: https://reviews.apache.org/r/61802/diff/1/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61758: GEODE-3471: Identify NPE in MBeanProxyFactory

[Jared Stewart]

> On Aug. 21, 2017, 5:55 p.m., Patrick Rhomberg wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/MBeanProxyFactory.java
> > Lines 83-84 (original), 81-85 (patched)
> > <https://reviews.apache.org/r/61758/diff/1/?file=1800306#file1800306line83>
> >
> > I might be missing context, but this just a spike to see which of these 
> > methods is returning `null`, yeah?
> > 
> > Is receiving `null` in this context indicative of failure, or should 
> > there be some null checks and handling?  Should we formally check against 
> > null and throw instead of allowing an NPE to bubble up?  Would that be a 
> > more meaningful error than an NPE?

This change is just to help see what's null.  Once we know, we'll be able to 
decide how to properly deal with a null value.


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61758/#review183354
-------


On Aug. 18, 2017, 9:55 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61758/
> ---
> 
> (Updated Aug. 18, 2017, 9:55 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3471: Identify NPE in MBeanProxyFactory
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/MBeanProxyFactory.java
>  0cce0be22323cf47dd6f90691bb068b3aaa77372 
> 
> 
> Diff: https://reviews.apache.org/r/61758/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: [Discuss] Compatibility with Apache Xerces

[Jared Stewart]

It sounds like these changes ought to be ok.  If there is no objection, I will 
go ahead and pull them in this afternoon.

Thanks,
Jared 

> On Aug 21, 2017, at 9:09 AM, Jacob Barrett  wrote:
> 
> Looking into the legacy repo I can see that the fix for the StringBuffer
> issue was not brought into Geode. The fix was to specifically request the
> Oracle/Sun implementation. Since that implementation might not be available
> in the IBM JDK its probably not a great fix.
> 
> 
> On Fri, Aug 18, 2017 at 9:28 PM Darren Foong  wrote:
> 
>>> You're right. This did come up with the IBM JDK and we fixed it. Not
>> sure why then it's coming up again.
>> 
>> Could it be this? https://issues.apache.org/jira/browse/GEODE-135
>> Seems like a different issue with the IBM JDK though.
>> 
>> I've just verified that the whitespace problem is still an issue with
>> the IBM JDK (because it uses Apache Xerces), and this pull request
>> fixes it.
>> 
>> Not sure why the reporter for GEODE-135 didn't face a similar problem back
>> then.
>> 
>> - Darren
>> 
>> On Sat, Aug 19, 2017 at 6:04 AM, Jacob Barrett 
>> wrote:
>>> You're right. This did come up with the IBM JDK and we fixed it. Not
>> sure why then it's coming up again.
>>> 
>>> Sent from my iPhone
>>> 
>>> On Aug 18, 2017, at 2:01 PM, Dan Smith  wrote:
>>> 
> Since the oracle parser is always going to be there I don't see any
>> harm
 in doing that.
 
 That's not true if we're running on non-oracle JDKs, right? I remember a
 while back someone was trying to run geode on IBMs JDK and having
>> issues -
 maybe even this same whitespace problem?
 
 I think it this fixes issues with other parsers it looks good to me, I
 don't have a problem with adding xerces as a test dependency.
 
 -Dan
 
> On Fri, Aug 18, 2017 at 10:48 AM, Jacob Barrett 
>> wrote:
> 
> I could have sworn at one point the the cache xml parser explicitly
> requested the oracle parser. Since the oracle parser is always going
>> to be
> there I don't see any harm in doing that.
> 
> A better fix might be to just normalize the white space when parsing.
> 
> I also recall xerces having a flag for controlling the white space
> treatment.
> 
> -Jake
> 
> 
> Sent from my iPhone
> 
>> On Aug 18, 2017, at 10:25 AM, Anilkumar Gingade 
> wrote:
>> 
>> Why worry is claiming to support multiple version; and trying to
>> manage/maintain it...
>> 
>> -Anil.
>> 
>> 
>> On Thu, Aug 17, 2017 at 11:35 PM, Darren Foong >> 
>> wrote:
>> 
>>> Hi all,
>>> 
>>> I'm using Geode in an application that uses the Apache implementation
>>> of Xerces. The Oracle JDK comes with its own implementation of
>> Xerces.
>>> 
>>> I encountered an issue
>>> (https://issues.apache.org/jira/browse/GEODE-3306) whereby cache.xml
>>> parsing fails with Apache Xerces; details are in JIRA.
>>> 
>>> Currently there are two workarounds:
>>> 
>>> 1. Remove the whitespace between elements in cache.xml
>>> 2. Load the JDK Xerces when parsing cache.xml
>>> 
>>> I've submitted a pull request
>>> (https://github.com/apache/geode/pull/668) to make `CacheXmlParser`
>>> compatible with both versions of Xerces.
>>> 
>>> This change would be useful for at least two groups of people:
>>> 
>>> 1. Developers who are using the Apache implementation of Xerces
>>> throughout their application, and only want one implementation of
>>> Xerces
>>> 2. Developers who are using a non-Oracle JDK
>>> 
>>> Does anyone have any objections to having `xercesImpl` as a test
>>> runtime dependency?
>>> 
>>> I'd appreciate any feedback. Thank you!
>>> 
>>> Best regards,
>>> - Darren Foong
>>> 
> 
>> 

Review Request 61757: GEODE-2859: Fix ShowDeadlockDUnitTest

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61757/
---

Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-2859: Fix ShowDeadlockDUnitTest


Diffs
-

  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
 8b5c80e4dd63d894bb305c618b9c12ca1e318b52 


Diff: https://reviews.apache.org/r/61757/diff/1/


Testing
---


Thanks,

Jared Stewart

Re: [DISCUSS] authorizing function execution

[Jared Stewart]

We already have a similar annotation that we use internally to secure GFSH 
commands: 
@ResourceOperation(resource = Resource.DATA, operation = Operation.MANAGE)
public Result createRegion(
One option might be to move this annotation into a public package and to 
enforce it if present on Functions.  The only caveat is that we would need to 
extend the annotation a bit to support multiple permissions (right now it only 
supports a single permission like "DATA:READ", but not e.g. "DATA:READ and 
CLUSTER:READ" ). In any case, the permissions required to execute a function 
should not change from what they are today unless a function author explicitly 
specifies the permissions that should be required.  (Whether in the form of an 
annotation, a method, or whatever form we decide on.)

- Jared
> On Aug 17, 2017, at 1:35 PM, Jacob Barrett  wrote:
> 
> Y'all should checkout PicketLink (now KeyCloak) and how they handle this. I
> used this heavily in the past with JBoss AS. It supports annotation and
> inline restrictions. Also supports EL but that's for another day. While I
> am not advocating today (maybe tomorrow) that we witch to PicketLink but
> looking at other projects that have solved with application server domain
> issue may help us make better choices.
> 
> 
> http://docs.jboss.org/picketlink/2/latest/reference/html/Checking_for_Permissions.html
> 
> 
> On Thu, Aug 17, 2017 at 1:27 PM Udo Kohlmeyer  wrote:
> 
>> I don't think we should give a user explicit permissions to change the
>> authorization levels for their function.
>> 
>> I think that the SecurityService should have some knowledge about
>> authorization levels on a per-user or per-role. That would check the
>> authorization level on a per-user basis for every function. This might
>> require a more fine grained authorization mechanism to be role-based or
>> more specific user authorization settings should be set up that certain
>> users can only execute specific functions, or not execute functions at
>> all...
>> 
>> In addition to that, there are many functions that could feasibly be
>> read-only. Thus "DATA:READ" permissions would be sufficient... but the
>> current system does not protect itself to access a region and execute a
>> "WRITE" operation, so we are left with the lowest common denominator.
>> 
>> --Udo
>> 
>> 
>> On 8/17/17 12:10, Swapnil Bawaskar wrote:
>>> So, it sounds like if we did #1 above: i.e:
>>> 1) externalize SecurityService so that function author can use it in the
>>> function.execute code to check authorization.
>>> 
>>> we could get this to work with lambdas:
>>> ResultCollector rc = getExecution().execute(context ->
>>>   context.getCache().getSecurityService().authorizeRead();
>>>   // perform read
>>>   context.getResultSender().lastResult(result)
>>> );
>>> 
>>> Udo,
>>> I think there are plenty of use cases where a user would only have
>>> privileges to read data, if this reading of data involves a function,
>> that
>>> user will need write privileges too.
>>> 
>>> On Thu, Aug 17, 2017 at 11:26 AM Udo Kohlmeyer 
>>> wrote:
>>> 
 In the case of lambda executions... we have no way to annotate the
 lambda...
 
 So maybe the "outer" service call needs to help us... Maybe the security
 framework should automatically prevent the execution of any function is
 the user does not have "DATA:READ"/"DATA:WRITE" or "FUNCTION:EXEC"
 privileges.
 
 In addition to this, do we need to distinguish between "read" and
 "write" functions?
 
 
 On 8/17/17 10:43, Dan Smith wrote:
>> if we get to Lambda-based functions
> No if about it, this works right now and we do this in our tests :)
> 
> ResultCollector rc = getExecution().execute(context ->
>context.getResultSender().lastResult("done")
> );
> 
> -Dan
> 
> 
> On Thu, Aug 17, 2017 at 9:59 AM, Udo Kohlmeyer 
> wrote:
> 
>> I think there might be more to this than just
>> "Data:READ"/"Data:WRITE".
>> Why would we not support something like
 /*@Authorize(hasRole("functionExecutor"))*/
>> or /*@Authorize(requiresPermission("DATA:READ"))*/
>> 
>> The next question in my mind would be, if we get to Lambda-based
>> functions, how do we specify authorization credentials then?
>> Annotations
>> are great to "static" definition on services, not so great for more
>> "dynamic" execution of stuff...
>> 
>> 
>> 
>> On 8/17/17 09:29, Dan Smith wrote:
>> 
>>> I like the annotation idea, especially considering that Jared was
 talking
>>> about adding a @RegisterableFunction annotation as well. maybe
 something
>>> like @ResourcePermission("Data:READ") or something like that?
>>> 
>>> -Dan
>>> 
>>> On Thu, Aug 17, 2017 at 8:18 AM, Michael William Dodge <
 mdo...@pivotal.io
>>> wrote:
>>> 
>>> What 

Re: Review Request 61694: GEODE-3235: Deploy jar registers functions which extend FunctionAdapter

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61694/
---

(Updated Aug. 17, 2017, 8:21 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Clean up imports


Repository: geode


Description
---

GEODE-3235: Deploy jar registers functions which extend FunctionAdapter
- Extract FunctionScanner class
- Add scanning for FunctionAdapter in addition to Function
- Add test to expose GEODE-3429


Diffs (updated)
-

  geode-core/src/main/java/org/apache/geode/internal/DeployedJar.java 037ef9e 
  
geode-core/src/main/java/org/apache/geode/management/internal/deployment/FunctionScanner.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandFunctionRegistrationDUnitTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/FunctionScannerTest.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/AbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/AbstractImplementsFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/AnnotatedFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ConcreteExtendsAbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ConcreteExtendsAbstractImplementsFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ExtendsAbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ImplementsFunction.java
 PRE-CREATION 
  geode-junit/src/main/java/org/apache/geode/test/compiler/JarBuilder.java 
beea476 
  geode-junit/src/main/java/org/apache/geode/test/compiler/JavaCompiler.java 
8449605 


Diff: https://reviews.apache.org/r/61694/diff/2/

Changes: https://reviews.apache.org/r/61694/diff/1-2/


Testing
---

precheckin running


Thanks,

Jared Stewart

Re: Changes to Jenkins geode-nightly build job

[Jared Stewart]

Thanks Jens, this will be a huge improvement!

> On Aug 17, 2017, at 10:35 AM, Jens Deppe  wrote:
> 
> Hi,
> 
> I'm making changes to the geode-nightly build such that it will use docker
> containers for distributedTests. This means that these tests can also be
> parallelized, although for now I'm setting the parallelism to 1.
> 
> Initially, this change should help with network isolation for tests so that
> bind exceptions will be reduced or completely eliminated. If this build
> becomes consistently green (or blue in Jenkins parlance) we can increase
> the parallelism and have the builds run faster.
> 
> This change will be effective for the next build.
> 
> --Jens

Re: Review Request 61701: GEODE-3277: Fix error path constructors of Launcher inner State classess

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61701/#review183145
---




geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
Lines 2017 (patched)
<https://reviews.apache.org/r/61701/#comment259161>

I think this double-ternary might be easier to read if we split it up into 
a few separate statements.



geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java
Lines 85 (patched)
<https://reviews.apache.org/r/61701/#comment259162>

Looks like you intended to uncomment this line.


- Jared Stewart


On Aug. 16, 2017, 9:21 p.m., Ken Howe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61701/
> ---
> 
> (Updated Aug. 16, 2017, 9:21 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, 
> and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Updated tests for changes in the error constructors for ServerState and
> LocatorState.
> 
> Minor spelling corrections.
> 
> I've updated the fix for GEODE-3277 and rebased on top of the @klund's recent 
> process controller updates
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java
>  3a98373938e3de21da6badcf460dae3648218ac6 
>   geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 
> 83c1ab533e3dea323a8a99f7002b9464a54dfc25 
>   geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 
> ae64691605130c9b212a3a33bb65ae37b28af02b 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java
>  dd5841f4cffca38da07a11f381cf4174d7264349 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
>  e7f17ef208a1708f385c7c4041affb70fd309a4c 
> 
> 
> Diff: https://reviews.apache.org/r/61701/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin is in progress.
> 
> 
> Thanks,
> 
> Ken Howe
> 
>

Review Request 61694: GEODE-3235: Deploy jar registers functions which extend FunctionAdapter

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61694/
---

Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3235: Deploy jar registers functions which extend FunctionAdapter
- Extract FunctionScanner class
- Add scanning for FunctionAdapter in addition to Function
- Add test to expose GEODE-3429


Diffs
-

  geode-core/src/main/java/org/apache/geode/internal/DeployedJar.java 
037ef9ee96f6a01927c6b2c429f186e4eab285be 
  
geode-core/src/main/java/org/apache/geode/management/internal/deployment/FunctionScanner.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandFunctionRegistrationDUnitTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/FunctionScannerTest.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/AbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/AbstractImplementsFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/AnnotatedFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ConcreteExtendsAbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ConcreteExtendsAbstractImplementsFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ExtendsAbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/ImplementsFunction.java
 PRE-CREATION 
  geode-junit/src/main/java/org/apache/geode/test/compiler/JarBuilder.java 
beea476d6a587bc0dcc434a03ea8b6beb53b449e 
  geode-junit/src/main/java/org/apache/geode/test/compiler/JavaCompiler.java 
844960522726981fd3895caf336aef27de462485 


Diff: https://reviews.apache.org/r/61694/diff/1/


Testing
---

precheckin running


Thanks,

Jared Stewart

Re: Review Request 61627: GEODE-3437: Fix list and describe region tests

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61627/
---

(Updated Aug. 16, 2017, 5:24 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Safe copy of Properties before we add to it


Repository: geode


Description
---

GEODE-3437: Fix list and describe region tests


Diffs (updated)
-

  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListAndDescribeRegionDUnitTest.java
 ab8c69b 
  
geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
 fc7966f 


Diff: https://reviews.apache.org/r/61627/diff/2/

Changes: https://reviews.apache.org/r/61627/diff/1-2/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Review Request 61627: GEODE-3437: Fix list and describe region tests

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61627/
---

Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3437: Fix list and describe region tests


Diffs
-

  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListAndDescribeRegionDUnitTest.java
 ab8c69b7cc99c88dd4e928efeb441d7d1a1d9b1b 
  
geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
 fc7966f5eb2a9ca4c30369a20ce664d3929ecc22 


Diff: https://reviews.apache.org/r/61627/diff/1/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: [DISCUSS] New annotation to identify Functions whose class hierarchy spans multiple jar files

[Jared Stewart]

Hi John,

Thanks for the suggestions. I like “@RegisterableFunction” better than 
“@RegisterFunction".  I think that we don’t want @RegisterableFunction to be 
@Inherited, in order to avoid creating a new variant of the problem we are 
trying to fix.  As you suggest, we should be mindful to document the behavior 
clearly. 

In case you’re interested, here’s 
<https://github.com/lukehutch/fast-classpath-scanner/wiki/3.4.-Finding-classes-with-specific-annotations-or-meta-annotations>
 some documentation of the API for the library we’re using for the annotation 
scanning.  It scans the binary byte code files directly, rather than using a 
Class reference like Spring utilities you pointed out (which AFAIK requires 
having that class loaded in the JVM).

Thanks,
Jared

> On Aug 11, 2017, at 11:00 AM, John Blum <jb...@pivotal.io> wrote:
> 
> Hi Jared-
> 
> In general, I like this idea since Annotations are a great form of
> meta-data and essentially meaningless outside of the intended context and
> therefore do not impose any adverse effects on any existing behavior.
> 
> However, 2 things... 1 suggestion and 1 caution...
> 
> 
> 1. Perhaps "RegisterableFunction" as opposed to "RegisterFunction".
> 
> 
> 2. Annotations can be "inherited" in the same way that your ConcreteFunction
> extends (inherits from) AbstractFunction, so too can a more concrete
> Function possibly inherit from a less-concrete-but-not-abstract Function.
> A developer might expect that they don't have to re-annotated his/her
> function.
> 
> For example...
> 
> @RegisterableFunction
> class ProcessOrder extends FunctionAdapter { ... }
> 
> class CreateAccountAtPointOfSale extends ProcessOrder { ... }
> 
> If these are in separate JAR files, depending on the "application", I may
> only want to "register" the CreateAccountAtPointOfSale Function and not the
> ProcessOrder Function explicitly.  Please forgive my example here since it
> seems like a bad example given it feels like 2 separate actions but is
> often part of the same workflow and so really depends on how application
> developers think about and organize their logic, which usually leads to
> "unexpected" things you never anticipated when introducing something like
> this.
> 
> SIDE NOTE: Of course, from a Java SE standpoint, the "RegisteredFunction"
> Annotation could be (but does not have to be) meta-annotated with @Inherited.,
> such as...
> 
> @Target(ElementType.TYPE)
> @Retention(RetentionPolicy.RUNTIME)
> @Inherited
> ...
> @interface RegisteredFunction { .. }
> 
> 
> *Spring* takes all of these things into account when it processes
> annotations of this nature (e.g. @Enable...).  Especially have a look at
> o.s.core.annotation.AnnotatedElementUtils [1] and even
> o.s.core.annotation.AnnotationUtils [2].  These are very robust and very
> powerful classes underpinning much of *Spring's* Annotation configuration
> and processing.  *Boot* also extends this functionality and *Spring*
> Annotation config in very specific/custom ways.
> 
> I think it is reasonable to set limitations to keep the initial scope
> small, but be sure those are well documented since users will be coming
> from many different frameworks having many different expectations.
> 
> Food for thought/hope this helps.
> 
> Regards,
> -John
> 
> 
> [1]
> http://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/core/annotation/AnnotatedElementUtils.html
> [2]
> http://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/core/annotation/AnnotationUtils.html
> 
> 
> 
> 
> 
> On Fri, Aug 11, 2017 at 10:22 AM, Jared Stewart <jstew...@pivotal.io> wrote:
> 
>> Recent changes introduced to avoid the eager loading of classes have lead
>> to Functions not getting registered correctly if the class hierarchy
>> leading up to Function (or FunctionAdapter) is split up across multiple jar
>> files.  We propose to introduce a new annotation to identify functions in
>> such a case.
>> Consider the following scenario:
>>> Abstract.jar - public abstract class AbstractFunction implements
>> Function {...}
>>> Concrete.jar - public class ConcreteFunction extends AbstractFunction
>> {...}
>> When Concrete.jar is deployed, we only scan the classes inside
>> Concrete.jar.  This means that we have no way of knowing that
>> AbstractFunction eventually leads up to Function.  (We could load
>> ConcreteFunction to see if it implements Function via reflection or
>> Function.class.isAssignableFrom(), but then we would be back to eagerly
>> loading all of the classes to see whether or not they imple

Re: Review Request 61599: GEODE-3328: fix a test failure on windows.

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61599/#review182759
---


Ship it!




Ship It!

- Jared Stewart


On Aug. 11, 2017, 10:42 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61599/
> ---
> 
> (Updated Aug. 11, 2017, 10:42 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3328: fix a test failure on windows.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GfshCommandJUnitTest.java
>  da60c7aa481954be0acc8c7e2b88717cf8bc9c37 
> 
> 
> Diff: https://reviews.apache.org/r/61599/diff/1/
> 
> 
> Testing
> ---
> 
> the test itself since only this test is changed.
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 61563: GEODE-3383: Refactor deploy tests

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61563/
---

(Updated Aug. 11, 2017, 9:52 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3383: Refactor deploy tests

- Refactor DeployedJarJUnitTest
- Move several tests into more appropriate locations


Diffs
-

  
geode-core/src/test/java/org/apache/geode/internal/ClassPathLoaderIntegrationTest.java
 34d8a2318422edbb3bbdc04920a41693f8d3fe69 
  geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java 
178dbae2eaada0cc054502fdf4b6d2ff102b4ed6 
  
geode-core/src/test/java/org/apache/geode/internal/JarDeployerDeadlockTest.java 
PRE-CREATION 
  geode-core/src/test/java/org/apache/geode/management/DeployJarTestSuite.java 
6dfab66926c7b246bf839632b293330959f1d728 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java
 89148d7752ae1f69e25671ffc43101a63cf7dc64 
  
geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpDUnitTest.java
 7753aafbd7dc5ea4288e27f088400163f6739347 


Diff: https://reviews.apache.org/r/61563/diff/1/


Testing (updated)
---

Precheckin passed


Thanks,

Jared Stewart

Review Request 61563: GEODE-3383: Refactor deploy tests

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61563/
---

Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3383: Refactor deploy tests

- Refactor DeployedJarJUnitTest
- Move several tests into more appropriate locations


Diffs
-

  
geode-core/src/test/java/org/apache/geode/internal/ClassPathLoaderIntegrationTest.java
 34d8a2318422edbb3bbdc04920a41693f8d3fe69 
  geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java 
178dbae2eaada0cc054502fdf4b6d2ff102b4ed6 
  
geode-core/src/test/java/org/apache/geode/internal/JarDeployerDeadlockTest.java 
PRE-CREATION 
  geode-core/src/test/java/org/apache/geode/management/DeployJarTestSuite.java 
6dfab66926c7b246bf839632b293330959f1d728 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java
 89148d7752ae1f69e25671ffc43101a63cf7dc64 
  
geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpDUnitTest.java
 7753aafbd7dc5ea4288e27f088400163f6739347 


Diff: https://reviews.apache.org/r/61563/diff/1/


Testing
---


Thanks,

Jared Stewart

Re: Review Request 61480: GEODE-3330: user needs CLUSTER:MANAGE:QUERY permission to create a CQ.

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61480/#review182442
---


Ship it!




Ship It!

- Jared Stewart


On Aug. 8, 2017, 9:10 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61480/
> ---
> 
> (Updated Aug. 8, 2017, 9:10 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * cq.execute() and cq.executeWithInitialResult() all would still require 
> DATA:READ because it will send the result back to the client either initially 
> or later.
> 
> 
> Diffs
> -
> 
>   geode-core/src/test/java/org/apache/geode/security/SecurityTestUtil.java 
> 5d5c2148e2eba8054df305942e04f43ea69c0a79 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
>  a455aff315cd26abd398630ff63d8b54b0d1d12b 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQ.java
>  6748f7d3193babfa668a7ff2846f974f0cdc1cbd 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61.java
>  77a608c7e65a2030c0037e9f327cf8c17e9313db 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/StopCQ.java
>  a3d51edcc141391e9d818fc0ed7e514d3cb5d6d0 
>   
> geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQTest.java
>  PRE-CREATION 
>   
> geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61Test.java
>  PRE-CREATION 
>   
> geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/StopCQTest.java
>  PRE-CREATION 
>   geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDUnitTest.java 
> 89167258ddbc06315068c849255a8530faefe060 
>   
> geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java
>  45f45abd17cf4f90f96163ebe4f01e67b3b53633 
>   geode-cq/src/test/java/org/apache/geode/security/ClientCQAuthDUnitTest.java 
> PRE-CREATION 
>   
> geode-cq/src/test/java/org/apache/geode/security/ClientQueryAuthDUnitTest.java
>  cc5dde409c561522ae3739eeaee892079c509ae8 
>   
> geode-cq/src/test/java/org/apache/geode/test/dunit/rules/CQUnitTestRule.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61480/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Review Request 61507: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61507/
---

Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3383: Refactor deploy tests

- Remove some duplicated tests
- Re-organize some tests
- DeployedJarJUnitTest now tests the public api of that class rather than 
implementation details


Diffs
-

  
geode-core/src/test/java/org/apache/geode/internal/ClassPathLoaderIntegrationTest.java
 34d8a2318422edbb3bbdc04920a41693f8d3fe69 
  geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java 
178dbae2eaada0cc054502fdf4b6d2ff102b4ed6 
  
geode-core/src/test/java/org/apache/geode/internal/JarDeployerDeadlockTest.java 
PRE-CREATION 
  geode-core/src/test/java/org/apache/geode/management/DeployJarTestSuite.java 
6dfab66926c7b246bf839632b293330959f1d728 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java
 89148d7752ae1f69e25671ffc43101a63cf7dc64 
  geode-junit/build.gradle e47095f5c538c4ae040b7135bf20eeef55bd77ba 
  
geode-junit/src/main/java/org/apache/geode/test/compiler/ClassNameExtractor.java
 PRE-CREATION 
  
geode-junit/src/main/java/org/apache/geode/test/compiler/CompiledSourceCode.java
 PRE-CREATION 
  geode-junit/src/main/java/org/apache/geode/test/compiler/JarBuilder.java 
PRE-CREATION 
  geode-junit/src/main/java/org/apache/geode/test/compiler/JavaCompiler.java 
PRE-CREATION 
  
geode-junit/src/main/java/org/apache/geode/test/compiler/UncompiledSourceCode.java
 PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/compiler/ClassNameExtractorTest.java
 PRE-CREATION 
  geode-junit/src/test/java/org/apache/geode/test/compiler/JarBuilderTest.java 
PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/compiler/JavaCompilerTest.java 
PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/compiler/UncompiledSourceCodeTest.java
 PRE-CREATION 
  
geode-junit/src/test/resources/org/apache/geode/test/compiler/AbstractClass.java
 PRE-CREATION 
  
geode-junit/src/test/resources/org/apache/geode/test/compiler/ConcreteClass.java
 PRE-CREATION 
  
geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpDUnitTest.java
 7753aafbd7dc5ea4288e27f088400163f6739347 


Diff: https://reviews.apache.org/r/61507/diff/1/


Testing
---


Thanks,

Jared Stewart

Re: Review Request 61480: GEODE-3330: user needs CLUSTER:MANAGE:QUERY permission to create a CQ.

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61480/#review182434
---




geode-cq/src/test/java/org/apache/geode/security/ClientCQAuthDUnitTest.java
Lines 54 (patched)
<https://reviews.apache.org/r/61480/#comment258340>

Oops, I was a little trigger happy with my "Ship it!".. Should there be one 
more test here to make sure that things work as expected when a user has both 
permissions?


- Jared Stewart


On Aug. 7, 2017, 8:13 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61480/
> ---
> 
> (Updated Aug. 7, 2017, 8:13 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * cq.execute() and cq.executeWithInitialResult() all would still require 
> DATA:READ because it will send the result back to the client either initially 
> or later.
> 
> 
> Diffs
> -
> 
>   geode-core/src/test/java/org/apache/geode/security/SecurityTestUtil.java 
> 5d5c2148e2eba8054df305942e04f43ea69c0a79 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
>  a455aff315cd26abd398630ff63d8b54b0d1d12b 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQ.java
>  6748f7d3193babfa668a7ff2846f974f0cdc1cbd 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61.java
>  77a608c7e65a2030c0037e9f327cf8c17e9313db 
>   geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDUnitTest.java 
> 89167258ddbc06315068c849255a8530faefe060 
>   
> geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java
>  45f45abd17cf4f90f96163ebe4f01e67b3b53633 
>   geode-cq/src/test/java/org/apache/geode/security/ClientCQAuthDUnitTest.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61480/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 61480: GEODE-3330: user needs CLUSTER:MANAGE:QUERY permission to create a CQ.

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61480/#review182433
---


Ship it!




Ship It!

- Jared Stewart


On Aug. 7, 2017, 8:13 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61480/
> ---
> 
> (Updated Aug. 7, 2017, 8:13 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * cq.execute() and cq.executeWithInitialResult() all would still require 
> DATA:READ because it will send the result back to the client either initially 
> or later.
> 
> 
> Diffs
> -
> 
>   geode-core/src/test/java/org/apache/geode/security/SecurityTestUtil.java 
> 5d5c2148e2eba8054df305942e04f43ea69c0a79 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
>  a455aff315cd26abd398630ff63d8b54b0d1d12b 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQ.java
>  6748f7d3193babfa668a7ff2846f974f0cdc1cbd 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61.java
>  77a608c7e65a2030c0037e9f327cf8c17e9313db 
>   geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDUnitTest.java 
> 89167258ddbc06315068c849255a8530faefe060 
>   
> geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java
>  45f45abd17cf4f90f96163ebe4f01e67b3b53633 
>   geode-cq/src/test/java/org/apache/geode/security/ClientCQAuthDUnitTest.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61480/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 61166: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61166/
---

(Updated Aug. 8, 2017, 5:14 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Improve error handling and remove unused test resources.


Repository: geode


Description
---

GEODE-3313: Test utility supports building jar files with multiple classes


Diffs (updated)
-

  geode-junit/build.gradle e47095f 
  
geode-junit/src/main/java/org/apache/geode/test/compiler/ClassNameExtractor.java
 PRE-CREATION 
  
geode-junit/src/main/java/org/apache/geode/test/compiler/CompiledSourceCode.java
 PRE-CREATION 
  geode-junit/src/main/java/org/apache/geode/test/compiler/JarBuilder.java 
PRE-CREATION 
  geode-junit/src/main/java/org/apache/geode/test/compiler/JavaCompiler.java 
PRE-CREATION 
  
geode-junit/src/main/java/org/apache/geode/test/compiler/UncompiledSourceCode.java
 PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/compiler/ClassNameExtractorTest.java
 PRE-CREATION 
  geode-junit/src/test/java/org/apache/geode/test/compiler/JarBuilderTest.java 
PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/compiler/JavaCompilerTest.java 
PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/compiler/UncompiledSourceCodeTest.java
 PRE-CREATION 
  
geode-junit/src/test/resources/org/apache/geode/test/compiler/AbstractClass.java
 PRE-CREATION 
  
geode-junit/src/test/resources/org/apache/geode/test/compiler/ConcreteClass.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61166/diff/8/

Changes: https://reviews.apache.org/r/61166/diff/7-8/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61480: GEODE-3330: user needs CLUSTER:MANAGE:QUERY permission to create a CQ.

[Jared Stewart]

> On Aug. 7, 2017, 8:21 p.m., Jared Stewart wrote:
> > geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61.java
> > Lines 140 (patched)
> > <https://reviews.apache.org/r/61480/diff/1/?file=1791025#file1791025line141>
> >
> > Can you clarify something for me?  The change looks like it's addding a 
> > check for CLUSTER:MANAGE:QUERY.  But from reading GEODE-3330, I thought we 
> > already checked for that and instead needed to add a check for 
> > DATA:READ:REGION.  But it looks like that might have already been happening 
> > on line 126 (122 of the original code).
> 
> Jinmei Liao wrote:
> line 126 is the authorization for old AccessControl interface, it does 
> nothing for the new integrated security. it was there in case user is still 
> using the old security model.
> 
> I was orignally confused as well. Turns out we are checking 
> DATA:READ:regionName already for both execute and executeWithInitialResult, 
> we are just adding the CLUSTER:MANAGE.QUERY check.

Would you mind pointing me to where the existing check happens for my own 
edification?  Thanks!


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61480/#review182317
---


On Aug. 7, 2017, 8:13 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61480/
> -----------
> 
> (Updated Aug. 7, 2017, 8:13 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * cq.execute() and cq.executeWithInitialResult() all would still require 
> DATA:READ because it will send the result back to the client either initially 
> or later.
> 
> 
> Diffs
> -
> 
>   geode-core/src/test/java/org/apache/geode/security/SecurityTestUtil.java 
> 5d5c2148e2eba8054df305942e04f43ea69c0a79 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
>  a455aff315cd26abd398630ff63d8b54b0d1d12b 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQ.java
>  6748f7d3193babfa668a7ff2846f974f0cdc1cbd 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61.java
>  77a608c7e65a2030c0037e9f327cf8c17e9313db 
>   geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDUnitTest.java 
> 89167258ddbc06315068c849255a8530faefe060 
>   
> geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java
>  45f45abd17cf4f90f96163ebe4f01e67b3b53633 
>   geode-cq/src/test/java/org/apache/geode/security/ClientCQAuthDUnitTest.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61480/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 61487: GEODE-3407: fix deadlock between JMX and reconnect

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61487/#review182405
---


Ship it!




Ship It!

- Jared Stewart


On Aug. 8, 2017, 4:38 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61487/
> ---
> 
> (Updated Aug. 8, 2017, 4:38 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3407
> https://issues.apache.org/jira/browse/GEODE-3407
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Change InternalClientMembership to not synchronize on CacheFactory
> by accepting Cache parameter from CacheServerBridge.
> 
> New regression test confirms bug and this fix.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/InternalClientMembership.java
>  504081d65515adb294dd43ecffee450477f08339 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/CacheServerBridge.java
>  728402c8a290d88026db753657e26e5f7440a990 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/ManagementAdapter.java
>  003a8f3cca4ff8fab031bdd84e0e360a14334f6b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/ServerBridge.java
>  6834998da13deec5e074a61e5373ec2f8dce2ad7 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/beans/CacheServerBridgeClientMembershipRegressionTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61487/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin in progress
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 61487: GEODE-3407: fix deadlock between JMX and reconnect

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61487/#review182347
---




geode-core/src/main/java/org/apache/geode/internal/cache/tier/InternalClientMembership.java
Line 292 (original), 292 (patched)
<https://reviews.apache.org/r/61487/#comment258218>

I might be missing something, but would callers of the no-arg variant 
`getClientQueueSizes()` potentially be prone to the same deadlock that 
originally affected `CacheServerBridge.getNumSubscriptions`?



geode-core/src/test/java/org/apache/geode/management/internal/beans/CacheServerBridgeClientMembershipRegressionTest.java
Lines 53 (patched)
<https://reviews.apache.org/r/61487/#comment258220>

I wonder if `startedExecuting` and `finishedExecuting` might make the 
intent of these variables clearer.



geode-core/src/test/java/org/apache/geode/management/internal/beans/CacheServerBridgeClientMembershipRegressionTest.java
Lines 91 (patched)
<https://reviews.apache.org/r/61487/#comment258219>

I really like this style of this test!



geode-core/src/test/java/org/apache/geode/management/internal/beans/CacheServerBridgeClientMembershipRegressionTest.java
Lines 97 (patched)
<https://reviews.apache.org/r/61487/#comment258222>

It might be nice to have a commment (or to wrap line this in a named 
method) to indicate that this line is expected to obtain the lock on 
`CacheFactory`.


- Jared Stewart


On Aug. 8, 2017, 12:19 a.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61487/
> ---
> 
> (Updated Aug. 8, 2017, 12:19 a.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3407
> https://issues.apache.org/jira/browse/GEODE-3407
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Change InternalClientMembership to not synchronize on CacheFactory
> by accepting Cache parameter from CacheServerBridge.
> 
> New regression test confirms bug and this fix.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/InternalClientMembership.java
>  504081d65515adb294dd43ecffee450477f08339 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/CacheServerBridge.java
>  728402c8a290d88026db753657e26e5f7440a990 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/ServerBridge.java
>  6834998da13deec5e074a61e5373ec2f8dce2ad7 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/beans/CacheServerBridgeClientMembershipRegressionTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61487/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin in progress
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 61472: GEODE-3097: fix an accidental bug introduced when working on ssl over http

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61472/#review182315
---


Ship it!




Ship It!

- Jared Stewart


On Aug. 7, 2017, 5:18 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61472/
> ---
> 
> (Updated Aug. 7, 2017, 5:18 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3097: fix an accidental bug introduced when working on ssl over http
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
>  be556a447d862144e453f69809a2de67ee00b654 
> 
> 
> Diff: https://reviews.apache.org/r/61472/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 61417: GEODE-3328: adding ssl-truststore-type to the config

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61417/#review182230
---


Ship it!




Ship It!

- Jared Stewart


On Aug. 3, 2017, 9:15 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61417/
> ---
> 
> (Updated Aug. 3, 2017, 9:15 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, 
> Patrick Rhomberg, and Udo Kohlmeyer.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3328: adding ssl-truststore-type to the config
> 
> this is what's requested in the JIRA ticket. This changeset just deals with 
> adding the property into gemfire properties
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
>  63f6505101f6edb62167b854d3d16d76d0a893cd 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java
>  795f6a5a4a4f42fe065c9ca6013fd5598f9311d8 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java
>  c2a395de0bfe21ed8efeb6b25e331f7085b3bf4f 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
>  fbe894c96447b2e30594eb2ed0652dd08e1028ce 
>   
> geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java
>  f86f07eb5e58b8509e909b7748795530efd65339 
>   geode-core/src/main/java/org/apache/geode/management/GemFireProperties.java 
> 08fa9b54ea066b4158478ae89d8295ed0b1a337b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/beans/BeanUtilFuncs.java
>  499ef010f354ebf88765190f1b5eb945da83accc 
>   
> geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
>  525f988cd3cb4f19872168df9b7105645f5c0498 
> 
> 
> Diff: https://reviews.apache.org/r/61417/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 61426: GEODE-3277: Fix error path constructors of inner State classes of the Launchers

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61426/#review182229
---


Ship it!




Ship It!

- Jared Stewart


On Aug. 4, 2017, 2:55 p.m., Ken Howe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61426/
> ---
> 
> (Updated Aug. 4, 2017, 2:55 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, 
> and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Updated tests for changes in the error constructors for ServerState and
> LocatorState.
> 
> Minor refactorings and spelling corrections to improve code clarity.
> 
> Some error results from gfsh 'status locator' and 'server loccator' commands 
> returned a messge of "null", or in some cases woudl throw a 
> NumberFormatException. These results were due to constructors for 
> LocatorLauncher.LocatorState and ServerLauncher.ServerState filling relevant 
> fields with nulls. This change sets the fields in the ...State instances to 
> values available through the command being executed. The affect constructors 
> are only used for building error results for the status commands.
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java
>  3a98373938e3de21da6badcf460dae3648218ac6 
>   geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 
> c5a2de88086e92dfc9b35d764b88ff8c8e524853 
>   geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 
> 158e7bf45a3cb72f6b96345810b935096b44ee7e 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/lifecycle/StatusLocatorCommand.java
>  06f835034c32e7c6cc7a11d9657d8b5d40d0f2d8 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/lifecycle/StatusServerCommand.java
>  43374ab161b67357d2f8b2987d7656156cbc12c1 
>   
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java
>  47e512a2dc3a8780dd941af8309865c4f1dbf36f 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
>  e7f17ef208a1708f385c7c4041affb70fd309a4c 
> 
> 
> Diff: https://reviews.apache.org/r/61426/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin ran green
> 
> 
> Thanks,
> 
> Ken Howe
> 
>

Re: Review Request 61409: GEODE-3328: simplify GfshParserRule

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61409/#review182225
---


Ship it!




I think this is a good improvement to the rule's API.

- Jared Stewart


On Aug. 3, 2017, 5:12 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61409/
> ---
> 
> (Updated Aug. 3, 2017, 5:12 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3328: simplify GfshParserRule
> 
> another step towards refactor connect command, some simple rule change.
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommandIntegrationTest.java
>  0700742fac70730c160d28c62c93b824e8ee0a3c 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StartServerCommandIntegrationTest.java
>  059611dc1c5101643cbae18d067a2943a573d405 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsTest.java
>  c2810801257479ad9a31452f294daaaf2975dfad 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshParserRule.java
>  b1bc7aa73b7d9273ad9a89af4c119d91a67aae03 
> 
> 
> Diff: https://reviews.apache.org/r/61409/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running, tests
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: behavior of "connect" command when --use-ssl

[Jared Stewart]

+1 to excluding the home dir at the very least.  I think that behavior is 
surprising, and it also makes it harder to maintain multiple separate, isolated 
versions of Geode on the same machine.  (One can easily introduce an unintended 
properties file into their configuration simply by copying it into their home 
directory.)

- Jared

> On Aug 3, 2017, at 3:30 PM, John Blum  wrote:
> 
> Good question.
> 
> +1 to... I also think specifying settings should be "explicit" rather than
> picking up arbitrary files in known locations  (e.g. working dir, home dir,
> classpath, etc).  This would be decidedly bad if an auth file were picked
> up that opened Geode up to the world, for instance.
> 
> On Thu, Aug 3, 2017 at 3:25 PM, jil...@pivotal.io  
> > wrote:
> 
>> Yeah, I noticed that too. It would be nice to have those other commands
>> NOT do these sort of things either. It is a change if behavior, but how
>> many users are using this behavior?
>> 
>> 
>> 
>>  Original Message 
>> Subject: Re: behavior of "connect" command when --use-ssl
>> From: John Blum
>> To: geode
>> CC:
>> 
>> 
>> Well, then `connect` will be inconsistent with other commands (e.g. `start
>> locator`) at best.
>> 
>> Geode is going to pick up the gfSecurity.properties file in your HOME
>> directory whether you want it to or not, and especially regardless of the
>> options given to either `start locator` or `start server` since Geode looks
>> in well known locations (work dir, HOME dir and CLASSPATH) for both
>> gemfire.properties and gfSecurity.properties, by default.
>> 
>> See here...
>> 
>> https://github.com/apache/geode/blob/rel/v1.2.0/geode-
>> core/src/main/java/org/apache/geode/distributed/internal/
>> DistributionConfigImpl.java#L864
>> 
>> Then here...
>> 
>> https://github.com/apache/geode/blob/rel/v1.2.0/geode-
>> core/src/main/java/org/apache/geode/distributed/
>> DistributedSystem.java#L687
>> 
>> And finally, here...
>> 
>> https://github.com/apache/geode/blob/rel/v1.2.0/geode-
>> core/src/main/java/org/apache/geode/distributed/
>> DistributedSystem.java#L690-L710
>> 
>> This...
>> 
>> https://github.com/apache/geode/blob/rel/v1.2.0/geode-
>> core/src/main/java/org/apache/geode/distributed/
>> DistributedSystem.java#L691-L693
>> 
>> ... is going to looking working directory (of the running GemFire
>> process... Locator/Server, Manager)
>> 
>> This...
>> 
>> https://github.com/apache/geode/blob/rel/v1.2.0/geode-
>> core/src/main/java/org/apache/geode/distributed/
>> DistributedSystem.java#L700-L702
>> 
>> ... checks the user's HOME dir, and finally...
>> 
>> This...
>> 
>> https://github.com/apache/geode/blob/rel/v1.2.0/geode-
>> core/src/main/java/org/apache/geode/distributed/
>> DistributedSystem.java#L709
>> 
>> ... resorts to resolving the [gemfire|gfSecurity].properties files from
>> the
>> CLASSPATH.
>> 
>> 
>> I am not opposed to the `connect` command changing in how it deals with
>> SSL, but it should be...
>> 
>> 1. Obvious to the user
>> 2. Consistent where it is not obvious.
>> 
>> $0.02
>> 
>> -John
>> 
>> 
>> 
>> On Thu, Aug 3, 2017 at 2:24 PM, Jinmei Liao wrote:
>> 
>>> I don't have any problem of having all the "security" info in another
>>> properties file, but the fact that it's still trying to load a property
>>> file even if the command did not say so explicitly. I might have such a
>>> file sitting in my home dir for some occasions, but I may not want to use
>>> it in this command. If I do want it to load a gfSecurity.properties, I
>>> would have just issued "connect
>>> --security-properties-file=gfSecurity.properties" instead. This feature
>> is
>>> there just to, in my opinion, save user a few key strokes in typing out
>> the
>>> command, but it can cause a lot of unnecessary confusion and
>> implementation
>>> hassle.
>>> 
>>> 
>>> On Thu, Aug 3, 2017 at 9:46 AM, Kirk Lund wrote:
>>> 
 Historically, gfSecurity.properties is a companion to
>> gemfire.properties
>>> in
 which sensitive key/value pairs can be kept in. The log banner does not
>>> (or
 did not) log any values in gfSecurity.properties. Users would also
 typically protect gfSecurity.properties with tighter permissions than
 gemfire.properties.
 
 At the same time SecurityLogWriter was introduced to the APIs (Cache,
 DistributedSystem). The idea behind this was that all security related
>>> log
 statements would go to a special log file with tighter permissions.
 
 I would prefer not having either gfSecurity.properties or
 SecurityLogWriter.
 Now that we use Log4J2 for logging, it would be pretty straightforward
>> to
 configure "security" loggers to log to a special log file without
>> having
>>> a
 dedicated SecurityLogWriter. As for gfSecurity.properties, we already
 redact all security related values from logging, so the only value of
 having it is that Users who have 

Re: Review Request 61166: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

> On Aug. 2, 2017, 4:44 p.m., Kirk Lund wrote:
> > You might consider hoisting these classes out of management pkg and into 
> > org.apache.geode.test.something. Maybe even move them to geode-test module. 
> > They seem very useful even outside of geode-core and management.
> > 
> > Ship It!

Great suggestion, this also lets me separate the test utilities from the tests 
that test the test utilities.  (That was a tongue twister..)


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61166/#review181999
---


On Aug. 2, 2017, 5:54 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61166/
> ---
> 
> (Updated Aug. 2, 2017, 5:54 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3313: Test utility supports building jar files with multiple classes
> 
> 
> Diffs
> -
> 
>   geode-junit/build.gradle e47095f 
>   
> geode-junit/src/main/java/org/apache/geode/test/compiler/ClassNameExtractor.java
>  PRE-CREATION 
>   
> geode-junit/src/main/java/org/apache/geode/test/compiler/CompiledSourceCode.java
>  PRE-CREATION 
>   geode-junit/src/main/java/org/apache/geode/test/compiler/JarBuilder.java 
> PRE-CREATION 
>   geode-junit/src/main/java/org/apache/geode/test/compiler/JavaCompiler.java 
> PRE-CREATION 
>   
> geode-junit/src/main/java/org/apache/geode/test/compiler/UncompiledSourceCode.java
>  PRE-CREATION 
>   
> geode-junit/src/test/java/org/apache/geode/test/compiler/ClassNameExtractorTest.java
>  PRE-CREATION 
>   
> geode-junit/src/test/java/org/apache/geode/test/compiler/JarBuilderTest.java 
> PRE-CREATION 
>   
> geode-junit/src/test/java/org/apache/geode/test/compiler/JavaCompilerTest.java
>  PRE-CREATION 
>   
> geode-junit/src/test/java/org/apache/geode/test/compiler/UncompiledSourceCodeTest.java
>  PRE-CREATION 
>   
> geode-junit/src/test/resources/org/apache/geode/test/compiler/AbstractExtendsFunctionAdapter.java
>  PRE-CREATION 
>   
> geode-junit/src/test/resources/org/apache/geode/test/compiler/AbstractFunction.java
>  PRE-CREATION 
>   
> geode-junit/src/test/resources/org/apache/geode/test/compiler/ConcreteExtendsAbstractExtendsFunctionAdapter.java
>  PRE-CREATION 
>   
> geode-junit/src/test/resources/org/apache/geode/test/compiler/ExtendsAbstractFunction.java
>  PRE-CREATION 
>   
> geode-junit/src/test/resources/org/apache/geode/test/compiler/ExtendsFunctionAdapter.java
>  PRE-CREATION 
>   
> geode-junit/src/test/resources/org/apache/geode/test/compiler/ImplementsFunction.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61166/diff/7/
> 
> 
> Testing
> ---
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 61166: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61166/
---

(Updated Aug. 2, 2017, 5:54 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Moved classes into geode-junit


Repository: geode


Description
---

GEODE-3313: Test utility supports building jar files with multiple classes


Diffs (updated)
-

  geode-junit/build.gradle e47095f 
  
geode-junit/src/main/java/org/apache/geode/test/compiler/ClassNameExtractor.java
 PRE-CREATION 
  
geode-junit/src/main/java/org/apache/geode/test/compiler/CompiledSourceCode.java
 PRE-CREATION 
  geode-junit/src/main/java/org/apache/geode/test/compiler/JarBuilder.java 
PRE-CREATION 
  geode-junit/src/main/java/org/apache/geode/test/compiler/JavaCompiler.java 
PRE-CREATION 
  
geode-junit/src/main/java/org/apache/geode/test/compiler/UncompiledSourceCode.java
 PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/compiler/ClassNameExtractorTest.java
 PRE-CREATION 
  geode-junit/src/test/java/org/apache/geode/test/compiler/JarBuilderTest.java 
PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/compiler/JavaCompilerTest.java 
PRE-CREATION 
  
geode-junit/src/test/java/org/apache/geode/test/compiler/UncompiledSourceCodeTest.java
 PRE-CREATION 
  
geode-junit/src/test/resources/org/apache/geode/test/compiler/AbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-junit/src/test/resources/org/apache/geode/test/compiler/AbstractFunction.java
 PRE-CREATION 
  
geode-junit/src/test/resources/org/apache/geode/test/compiler/ConcreteExtendsAbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-junit/src/test/resources/org/apache/geode/test/compiler/ExtendsAbstractFunction.java
 PRE-CREATION 
  
geode-junit/src/test/resources/org/apache/geode/test/compiler/ExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-junit/src/test/resources/org/apache/geode/test/compiler/ImplementsFunction.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61166/diff/7/

Changes: https://reviews.apache.org/r/61166/diff/6-7/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61166: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61166/
---

(Updated Aug. 1, 2017, 6:31 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Fix broken test


Repository: geode


Description
---

GEODE-3313: Test utility supports building jar files with multiple classes


Diffs (updated)
-

  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractor.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractorTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/CompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilder.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilderTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompiler.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompilerTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/UncompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/UncompiledSourceCodeTest.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ConcreteExtendsAbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsAbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ImplementsFunction.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61166/diff/6/

Changes: https://reviews.apache.org/r/61166/diff/5-6/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61231: GEODE-3328: simply extract connect command from ShellCommand

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61231/#review181866
---


Ship it!




Ship It!

- Jared Stewart


On July 28, 2017, 8:41 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61231/
> ---
> 
> (Updated July 28, 2017, 8:41 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> I am working on GEM-1575 which involves prompting for ssl configuration 
> properties when connecting to the cluster. So I refactoring the 
> ConnectCommand before doing that.
> This is the first step: simply extracting the command to its separate class 
> without any refactor at all
> 
> GEODE-3328: simply extract connect command from ShellCommand
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/util/PasswordUtil.java 
> ac0b8459e1cd8f399eed5e98bd9d5fbd4953f9c3 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java
>  d5e1b27715420818e9a63c9bd467d771e5ee20e3 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java
>  10dc0db871898b4fe95027baeacfc72f0e16d4a3 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/util/GfshConsoleReader.java
>  80a20e6d075f2b7c9202df96f36aa4c99c445e04 
>   
> geode-core/src/test/java/org/apache/geode/internal/util/PasswordUtilJUnitTest.java
>  8051c5671650ad4f4756f9f614778d1ae1387fa7 
> 
> 
> Diff: https://reviews.apache.org/r/61231/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 61166: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61166/
---

(Updated July 31, 2017, 10:37 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

- Add method for building a jar from class names alone
- Include a package with test/resources java files
- Use a system temp directory rather than a user-specified temp dir


Repository: geode


Description
---

GEODE-3313: Test utility supports building jar files with multiple classes


Diffs (updated)
-

  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractor.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractorTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/CompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilder.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilderTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompiler.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompilerTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/UncompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/UncompiledSourceCodeTest.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ConcreteExtendsAbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsAbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ImplementsFunction.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61166/diff/5/

Changes: https://reviews.apache.org/r/61166/diff/4-5/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61166: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

> On July 30, 2017, 5:05 a.m., Jinmei Liao wrote:
> > geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilder.java
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/61166/diff/4/?file=1785455#file1785455line66>
> >
> > looks like only the JavaCompiler needs a temp dir to store some 
> > temporary files and delete them afterwards. JarBuilder doesn't need it. Can 
> > we have JavaCompiler uses a system temp folder instead of needing a dir to 
> > construct it?

My concern was that the built-in `File.createTempFile` method doesn't clean up 
after itself as reliably as the JUnit `TemporaryFolder` rule.  I'll add a 
no-arg constructor that uses a system temp folder and also sets `deleteOnExit`.


> On July 30, 2017, 5:05 a.m., Jinmei Liao wrote:
> > geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilderTest.java
> > Lines 105 (patched)
> > <https://reviews.apache.org/r/61166/diff/4/?file=1785456#file1785456line105>
> >
> > this method seems useful, can we allow user to simply pass a className 
> > (with package prefixes) instead of a File object to the JarBuilder?
> > 
> > Basically I am wondering if the usage of the JarBuilder can be simplied 
> > to the following:
> > 
> > JarBuilder jarbuilder = new JarBuilder();
> > File outputFile = new File(tempDir, output.jar);
> > jarBuilder.buildJar(outputFile, "com.foo.Bar1", "com.foo.Bar2")

Great suggestion!  I will add this functionality.


> On July 30, 2017, 5:05 a.m., Jinmei Liao wrote:
> > geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsFunctionAdapter.java
> > Lines 19 (patched)
> > <https://reviews.apache.org/r/61166/diff/4/?file=1785464#file1785464line19>
> >
> > this has no package declaration. Is it intended?

I omitted a package declaration for simplicity since it didn't seem necessary 
to specify one.  I can add one in if you think the lack of a package is 
confusing.


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61166/#review181770
---


On July 28, 2017, 10:46 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61166/
> ---
> 
> (Updated July 28, 2017, 10:46 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3313: Test utility supports building jar files with multiple classes
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractor.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractorTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/CompiledSourceCode.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilder.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilderTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompiler.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompilerTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/UncompiledSourceCode.java
>  PRE-CREATION 
>   
> geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractExtendsFunctionAdapter.java
>  PRE-CREATION 
>   
> geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractFunction.java
>  PRE-CREATION 
>   
> geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ConcreteExtendsAbstractExtendsFunctionAdapter.java
>  PRE-CREATION 
>   
> geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsAbstractFunction.java
>  PRE-CREATION 
>   
> geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsFunctionAdapter.java
>  PRE-CREATION 
>   
> geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ImplementsFunction.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61166/diff/4/
> 
> 
> Testing
> ---
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 61166: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61166/
---

(Updated July 28, 2017, 10:46 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Added javadoc to explain usage. 
Fixed flaky test.


Repository: geode


Description
---

GEODE-3313: Test utility supports building jar files with multiple classes


Diffs (updated)
-

  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractor.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractorTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/CompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilder.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilderTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompiler.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompilerTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/UncompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ConcreteExtendsAbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsAbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ImplementsFunction.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61166/diff/4/

Changes: https://reviews.apache.org/r/61166/diff/3-4/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Review Request 61224: GEODE-3318: Fix UniversalMembershipListenerAdapterDUnitTest

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61224/
---

Review request for geode, Emily Yeh, Galen O'Sullivan, Jared Stewart, Ken Howe, 
Kirk Lund, and Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3318: Fix UniversalMembershipListenerAdapterDUnitTest


Diffs
-

  
geode-core/src/test/java/org/apache/geode/management/UniversalMembershipListenerAdapterDUnitTest.java
 d6f25792ce3a0f540b5722c4dfe3ad7de2d142a9 


Diff: https://reviews.apache.org/r/61224/diff/1/


Testing
---


Thanks,

Jared Stewart

Re: Review Request 61166: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61166/
---

(Updated July 27, 2017, 11:29 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Add missing license headers


Repository: geode


Description
---

GEODE-3313: Test utility supports building jar files with multiple classes


Diffs (updated)
-

  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractor.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractorTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/CompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilder.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilderTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompiler.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompilerTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/UncompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ConcreteExtendsAbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsAbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ImplementsFunction.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61166/diff/3/

Changes: https://reviews.apache.org/r/61166/diff/2-3/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61166: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61166/
---

(Updated July 27, 2017, 11:15 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Updated to remove deeply nested for loops


Repository: geode


Description
---

GEODE-3313: Test utility supports building jar files with multiple classes


Diffs (updated)
-

  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractor.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractorTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/CompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilder.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilderTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompiler.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompilerTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/UncompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ConcreteExtendsAbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsAbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ImplementsFunction.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61166/diff/2/

Changes: https://reviews.apache.org/r/61166/diff/1-2/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Review Request 61196: GEODE-3326: Fix intermittent ConcurrentDeployDUnitTest failure

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61196/
---

Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3326: Fix intermittent ConcurrentDeployDUnitTest failure


Diffs
-

  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ConcurrentDeployDUnitTest.java
 559440c8a2f227de092e52979d9db1f959a5d75f 


Diff: https://reviews.apache.org/r/61196/diff/1/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61185: GEODE-3231: use tempWorkingFolder to avoid test log file contamination between tests.

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61185/#review181613
---


Ship it!




Ship It!

- Jared Stewart


On July 27, 2017, 4:57 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61185/
> ---
> 
> (Updated July 27, 2017, 4:57 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3231: use tempWorkingFolder to avoid test log file contamination 
> between tests.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsWithMemberGroupDUnitTest.java
>  ef62269133a9842b81bfaa661b77ed80ddf8552d 
> 
> 
> Diff: https://reviews.apache.org/r/61185/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Review Request 61166: GEODE-3313: Test utility supports building jar files with multiple classes

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61166/
---

Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3313: Test utility supports building jar files with multiple classes


Diffs
-

  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractor.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/ClassNameExtractorTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/CompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilder.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JarBuilderTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompiler.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/JavaCompilerTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/deployment/compiler/UncompiledSourceCode.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/AbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ConcreteExtendsAbstractExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsAbstractFunction.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ExtendsFunctionAdapter.java
 PRE-CREATION 
  
geode-core/src/test/resources/org/apache/geode/management/internal/deployment/compiler/ImplementsFunction.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61166/diff/1/


Testing
---

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 61119: GEODE-3097: GFSH works over HTTP with SSL

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61119/#review181398
---


Ship it!




Ship It!

- Jared Stewart


On July 25, 2017, 10:11 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61119/
> ---
> 
> (Updated July 25, 2017, 10:11 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3097: GFSH works over HTTP with SSL
> 
> put the overhttp change in another test class and have it launhced in 
> geode-assembly, since only in that module the war file is created.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java
>  2da95a7c3f305aab4e615d4be7b14c19b9b31dbc 
>   
> geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java
>  844e0322eecdc93c4d46c546f4df2d278c5f15cd 
>   
> geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorWithLegacySSLDUnitTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
>  df371d28eac9d03cfb69ef462daf56f86bb95738 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectToLocatorSSLOverHttpTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61119/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin successful
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Review Request 61114: GEODE-3296: Speed up Acceptance Tests

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61114/
---

Review request for geode, Anthony Baker, Emily Yeh, Jens Deppe, Jared Stewart, 
Ken Howe, Kirk Lund, and Patrick Rhomberg.


Repository: geode


Description
---

GEODE-3296: Speed up Acceptance Tests

- Move the acceptanceTest task from the top-level module into the 
:geode-assembly submodule.


Diffs
-

  geode-assembly/build.gradle 02c9e33a3a35c423ff3d6aa9b1e151a34aca0378 
  gradle/test.gradle 741c64e8dc5d9965e03744b0929d7a701abb2456 


Diff: https://reviews.apache.org/r/61114/diff/1/


Testing
---

Precheckin running


Thanks,

Jared Stewart