[GitHub] incubator-geode issue #283: GEODE-2098: Moved gfsh history file location fro...

[davinash]

Github user davinash commented on the issue:

https://github.com/apache/incubator-geode/pull/283
  
Thanks @kirklund, @metatype I have followed the process from 
https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
Please do let me know if everything is correct.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode pull request #286: GEODE-2104: Fix parsing of options follow...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/incubator-geode/pull/286


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Re: gfsh start server with the --password option

[Karen Miller]

Also, when doing a gfsh connect (not just start server) that specifies user
and password
on the command line, if a further command of
  gfsh history --file=historyfilename
is executed, the user and password are written in clear text to the history
file.


On Tue, Nov 15, 2016 at 12:31 PM, Jinmei Liao  wrote:

> I thought we had code that deals with redacting password in gfsh history,
> not sure why it's not in effect anymore.
>
> On Tue, Nov 15, 2016 at 2:27 PM, Swapnil Bawaskar 
> wrote:
>
> > When you want to connect to a secure system you can choose not to use the
> > --password option at which point you will be prompted to enter a
> > username/password.
> > e.g:
> > gfsh>connect --locator=localhost[10334]
> > Connecting to Locator at [host=localhost, port=10334] ..
> > Connecting to Manager at [host=192.168.1.181, port=1099] ..
> > username: super-user
> > password: 
> >
> >
> > On Tue, Nov 15, 2016 at 11:55 AM, Kirk Lund  wrote:
> >
> > > There should be redaction in gfsh history. Maybe repeating the command
> > is a
> > > case that wasn't fully covered? This is a bug we'll need to file and
> fix.
> > >
> > > Clear text in process string is probably not a bug. Users should
> > implement
> > > a callback to provide the password instead of providing it as a system
> > > property unless they're ok with it showing in the process string. This
> > may
> > > need more documentation?
> > >
> > > The logs should not contain the clear text password and this would be a
> > bug
> > > if it does.
> > >
> > > -Kirk
> > >
> > >
> > > On Tue, Nov 15, 2016 at 11:08 AM, Karen Miller 
> > wrote:
> > >
> > > > When specifying user name and password to use as authentication
> > > credentials
> > > > with the gfsh start server command, the password is specified in the
> > > clear.
> > > > I've added a note in the documentation to point this out, but
> > specifying
> > > a
> > > > password
> > > > in this way leads to further ways the clear text password can be
> seen.
> > > >
> > > > - gfsh history will repeat back the command with the password shown
> > > > - any user on the box can see the clear text password with 'ps'
> > > > - (haven't checked if this happens) logs may have the clear text
> > password
> > > >
> > > > Is this an issue?  The history is for a particular user, so not so
> bad.
> > > > Logs can use file system permissions to reduce access.  But anyone
> with
> > > > access to the box can list the processes.
> > > >
> > > > Karen
> > > >
> > >
> >
>
>
>
> --
> Cheers
>
> Jinmei
>

Re: Review Request 53784: GEODE-1247: unable to stop server using http connection

[Kevin Duling]

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


Ship it!




Ship It!

- Kevin Duling


On Nov. 15, 2016, 7:06 a.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53784/
> ---
> 
> (Updated Nov. 15, 2016, 7:06 a.m.)
> 
> 
> Review request for geode, Jens Deppe, John Blum, Kevin Duling, and Kirk Lund.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-1247: unable to stop server using http connection
> 
> I believe after we added the "com.fasterxml.jackson.databind.ObjectMapper" 
> into core, the a mapper is added to the RestTemplate to precedes our own 
> message converter (SerializableObjectHttpMessageConverter), so on the client 
> side, we are using Jackson's converter to convert the message, but on the 
> server side, we are using our own converter, this caused the message corrupt 
> error. Fix is to remove the Jackson's converter from the client side.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/util/IOUtils.java 
> 4c1cc5f79a3bed922347835441f6624931bdcc6b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java
>  25eee8295ac553da416eb928b160fe7897b17763 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53784/diff/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 53784: GEODE-1247: unable to stop server using http connection

[Kirk Lund]

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


Ship it!




Ship It!

- Kirk Lund


On Nov. 15, 2016, 3:06 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53784/
> ---
> 
> (Updated Nov. 15, 2016, 3:06 p.m.)
> 
> 
> Review request for geode, Jens Deppe, John Blum, Kevin Duling, and Kirk Lund.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-1247: unable to stop server using http connection
> 
> I believe after we added the "com.fasterxml.jackson.databind.ObjectMapper" 
> into core, the a mapper is added to the RestTemplate to precedes our own 
> message converter (SerializableObjectHttpMessageConverter), so on the client 
> side, we are using Jackson's converter to convert the message, but on the 
> server side, we are using our own converter, this caused the message corrupt 
> error. Fix is to remove the Jackson's converter from the client side.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/util/IOUtils.java 
> 4c1cc5f79a3bed922347835441f6624931bdcc6b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java
>  25eee8295ac553da416eb928b160fe7897b17763 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53784/diff/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 53784: GEODE-1247: unable to stop server using http connection

[Jinmei Liao]

> On Nov. 15, 2016, 4:43 p.m., Kevin Duling wrote:
> > Why would we move away from using Jackson in favor of our own JSON mapping 
> > code?  Or is this a special case where we do some magic serialization we 
> > can't do via Jackson?
> 
> Jinmei Liao wrote:
> I tried using Jackson's converter, but it's giving errors about no 
> default constructor on one of the object it's trying to convert to. 
> Apparently, Jackson's converter requires the object either have some 
> annotation or a no-param constructor in order for it to work, but our own 
> doesn't.
> 
> Kirk Lund wrote:
> Is this temporary? We want to eventually delete the non-Jackson JSON 
> parser and change our code to use Jackson everywhere.

yeah, I believe we have that story somewhere. This is just to fix this error. 
Don't want to go down that rabbit hole.


- Jinmei


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


On Nov. 15, 2016, 3:06 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53784/
> ---
> 
> (Updated Nov. 15, 2016, 3:06 p.m.)
> 
> 
> Review request for geode, Jens Deppe, John Blum, Kevin Duling, and Kirk Lund.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-1247: unable to stop server using http connection
> 
> I believe after we added the "com.fasterxml.jackson.databind.ObjectMapper" 
> into core, the a mapper is added to the RestTemplate to precedes our own 
> message converter (SerializableObjectHttpMessageConverter), so on the client 
> side, we are using Jackson's converter to convert the message, but on the 
> server side, we are using our own converter, this caused the message corrupt 
> error. Fix is to remove the Jackson's converter from the client side.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/util/IOUtils.java 
> 4c1cc5f79a3bed922347835441f6624931bdcc6b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java
>  25eee8295ac553da416eb928b160fe7897b17763 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53784/diff/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 53784: GEODE-1247: unable to stop server using http connection

[Kirk Lund]

> On Nov. 15, 2016, 4:43 p.m., Kevin Duling wrote:
> > Why would we move away from using Jackson in favor of our own JSON mapping 
> > code?  Or is this a special case where we do some magic serialization we 
> > can't do via Jackson?
> 
> Jinmei Liao wrote:
> I tried using Jackson's converter, but it's giving errors about no 
> default constructor on one of the object it's trying to convert to. 
> Apparently, Jackson's converter requires the object either have some 
> annotation or a no-param constructor in order for it to work, but our own 
> doesn't.

Is this temporary? We want to eventually delete the non-Jackson JSON parser and 
change our code to use Jackson everywhere.


- Kirk


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


On Nov. 15, 2016, 3:06 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53784/
> ---
> 
> (Updated Nov. 15, 2016, 3:06 p.m.)
> 
> 
> Review request for geode, Jens Deppe, John Blum, Kevin Duling, and Kirk Lund.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-1247: unable to stop server using http connection
> 
> I believe after we added the "com.fasterxml.jackson.databind.ObjectMapper" 
> into core, the a mapper is added to the RestTemplate to precedes our own 
> message converter (SerializableObjectHttpMessageConverter), so on the client 
> side, we are using Jackson's converter to convert the message, but on the 
> server side, we are using our own converter, this caused the message corrupt 
> error. Fix is to remove the Jackson's converter from the client side.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/util/IOUtils.java 
> 4c1cc5f79a3bed922347835441f6624931bdcc6b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java
>  25eee8295ac553da416eb928b160fe7897b17763 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53784/diff/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 53784: GEODE-1247: unable to stop server using http connection

[Jinmei Liao]

> On Nov. 15, 2016, 4:43 p.m., Kevin Duling wrote:
> > Why would we move away from using Jackson in favor of our own JSON mapping 
> > code?  Or is this a special case where we do some magic serialization we 
> > can't do via Jackson?

I tried using Jackson's converter, but it's giving errors about no default 
constructor on one of the object it's trying to convert to. Apparently, 
Jackson's converter requires the object either have some annotation or a 
no-param constructor in order for it to work, but our own doesn't.


- Jinmei


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


On Nov. 15, 2016, 3:06 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53784/
> ---
> 
> (Updated Nov. 15, 2016, 3:06 p.m.)
> 
> 
> Review request for geode, Jens Deppe, John Blum, Kevin Duling, and Kirk Lund.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-1247: unable to stop server using http connection
> 
> I believe after we added the "com.fasterxml.jackson.databind.ObjectMapper" 
> into core, the a mapper is added to the RestTemplate to precedes our own 
> message converter (SerializableObjectHttpMessageConverter), so on the client 
> side, we are using Jackson's converter to convert the message, but on the 
> server side, we are using our own converter, this caused the message corrupt 
> error. Fix is to remove the Jackson's converter from the client side.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/util/IOUtils.java 
> 4c1cc5f79a3bed922347835441f6624931bdcc6b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java
>  25eee8295ac553da416eb928b160fe7897b17763 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53784/diff/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

[GitHub] incubator-geode issue #286: GEODE-2104: Fix parsing of options following --J

[jinmeiliao]

Github user jinmeiliao commented on the issue:

https://github.com/apache/incubator-geode/pull/286
  
LGTM. build it and start up gfsh and play with it with different options 
and see if it's not having some side effects. After that I can pull this in.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Re: gfsh start server with the --password option

[Jinmei Liao]

I thought we had code that deals with redacting password in gfsh history,
not sure why it's not in effect anymore.

On Tue, Nov 15, 2016 at 2:27 PM, Swapnil Bawaskar 
wrote:

> When you want to connect to a secure system you can choose not to use the
> --password option at which point you will be prompted to enter a
> username/password.
> e.g:
> gfsh>connect --locator=localhost[10334]
> Connecting to Locator at [host=localhost, port=10334] ..
> Connecting to Manager at [host=192.168.1.181, port=1099] ..
> username: super-user
> password: 
>
>
> On Tue, Nov 15, 2016 at 11:55 AM, Kirk Lund  wrote:
>
> > There should be redaction in gfsh history. Maybe repeating the command
> is a
> > case that wasn't fully covered? This is a bug we'll need to file and fix.
> >
> > Clear text in process string is probably not a bug. Users should
> implement
> > a callback to provide the password instead of providing it as a system
> > property unless they're ok with it showing in the process string. This
> may
> > need more documentation?
> >
> > The logs should not contain the clear text password and this would be a
> bug
> > if it does.
> >
> > -Kirk
> >
> >
> > On Tue, Nov 15, 2016 at 11:08 AM, Karen Miller 
> wrote:
> >
> > > When specifying user name and password to use as authentication
> > credentials
> > > with the gfsh start server command, the password is specified in the
> > clear.
> > > I've added a note in the documentation to point this out, but
> specifying
> > a
> > > password
> > > in this way leads to further ways the clear text password can be seen.
> > >
> > > - gfsh history will repeat back the command with the password shown
> > > - any user on the box can see the clear text password with 'ps'
> > > - (haven't checked if this happens) logs may have the clear text
> password
> > >
> > > Is this an issue?  The history is for a particular user, so not so bad.
> > > Logs can use file system permissions to reduce access.  But anyone with
> > > access to the box can list the processes.
> > >
> > > Karen
> > >
> >
>



-- 
Cheers

Jinmei

Re: gfsh start server with the --password option

[Swapnil Bawaskar]

When you want to connect to a secure system you can choose not to use the
--password option at which point you will be prompted to enter a
username/password.
e.g:
gfsh>connect --locator=localhost[10334]
Connecting to Locator at [host=localhost, port=10334] ..
Connecting to Manager at [host=192.168.1.181, port=1099] ..
username: super-user
password: 


On Tue, Nov 15, 2016 at 11:55 AM, Kirk Lund  wrote:

> There should be redaction in gfsh history. Maybe repeating the command is a
> case that wasn't fully covered? This is a bug we'll need to file and fix.
>
> Clear text in process string is probably not a bug. Users should implement
> a callback to provide the password instead of providing it as a system
> property unless they're ok with it showing in the process string. This may
> need more documentation?
>
> The logs should not contain the clear text password and this would be a bug
> if it does.
>
> -Kirk
>
>
> On Tue, Nov 15, 2016 at 11:08 AM, Karen Miller  wrote:
>
> > When specifying user name and password to use as authentication
> credentials
> > with the gfsh start server command, the password is specified in the
> clear.
> > I've added a note in the documentation to point this out, but specifying
> a
> > password
> > in this way leads to further ways the clear text password can be seen.
> >
> > - gfsh history will repeat back the command with the password shown
> > - any user on the box can see the clear text password with 'ps'
> > - (haven't checked if this happens) logs may have the clear text password
> >
> > Is this an issue?  The history is for a particular user, so not so bad.
> > Logs can use file system permissions to reduce access.  But anyone with
> > access to the box can list the processes.
> >
> > Karen
> >
>

[GitHub] incubator-geode issue #285: GEODE-1617: Regions can be created with a variet...

[kirklund]

Github user kirklund commented on the issue:

https://github.com/apache/incubator-geode/pull/285
  
Pushed to develop. Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Re: gfsh start server with the --password option

[Kirk Lund]

There should be redaction in gfsh history. Maybe repeating the command is a
case that wasn't fully covered? This is a bug we'll need to file and fix.

Clear text in process string is probably not a bug. Users should implement
a callback to provide the password instead of providing it as a system
property unless they're ok with it showing in the process string. This may
need more documentation?

The logs should not contain the clear text password and this would be a bug
if it does.

-Kirk


On Tue, Nov 15, 2016 at 11:08 AM, Karen Miller  wrote:

> When specifying user name and password to use as authentication credentials
> with the gfsh start server command, the password is specified in the clear.
> I've added a note in the documentation to point this out, but specifying a
> password
> in this way leads to further ways the clear text password can be seen.
>
> - gfsh history will repeat back the command with the password shown
> - any user on the box can see the clear text password with 'ps'
> - (haven't checked if this happens) logs may have the clear text password
>
> Is this an issue?  The history is for a particular user, so not so bad.
> Logs can use file system permissions to reduce access.  But anyone with
> access to the box can list the processes.
>
> Karen
>

[GitHub] incubator-geode pull request #286: GEODE-2104: Fix parsing of options follow...

[jaredjstewart]

GitHub user jaredjstewart opened a pull request:

https://github.com/apache/incubator-geode/pull/286

GEODE-2104: Fix parsing of options following --J



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jaredjstewart/incubator-geode 
feature/GEODE-2104

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-geode/pull/286.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #286


commit b4f1596bac584a1e0a82cb4a1065df5f496f1ee9
Author: Jared Stewart 
Date:   2016-11-15T18:57:11Z

GEODE-2104: Fix parsing of options following --J




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

gfsh start server with the --password option

[Karen Miller]

When specifying user name and password to use as authentication credentials
with the gfsh start server command, the password is specified in the clear.
I've added a note in the documentation to point this out, but specifying a
password
in this way leads to further ways the clear text password can be seen.

- gfsh history will repeat back the command with the password shown
- any user on the box can see the clear text password with 'ps'
- (haven't checked if this happens) logs may have the clear text password

Is this an issue?  The history is for a particular user, so not so bad.
Logs can use file system permissions to reduce access.  But anyone with
access to the box can list the processes.

Karen

[GitHub] incubator-geode pull request #285: GEODE-1617: Regions can be created with a...

[kjduling]

Github user kjduling commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/285#discussion_r88081170
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/RegionNameValidationJUnitTest.java
 ---
@@ -0,0 +1,75 @@
+package org.apache.geode.cache;
+
+import static org.junit.Assert.fail;
+
+import org.apache.geode.internal.cache.InternalRegionArguments;
+import org.apache.geode.internal.cache.LocalRegion;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+@Category(UnitTest.class)
+public class RegionNameValidationJUnitTest {
+  private static final Pattern NAME_PATTERN = 
Pattern.compile("[aA-zZ0-9-_.]+");
+  private static final String REGION_NAME = "MyRegion";
+
+
+  @Test
+  public void testInvalidNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+try {
+  LocalRegion.validateRegionName(null, ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+  LocalRegion.validateRegionName("", ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+  LocalRegion.validateRegionName("FOO" + Region.SEPARATOR, ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+
+  }
+
+  @Test
+  public void testExternalRegionNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+validateCharacters(ira);
+try {
+  LocalRegion.validateRegionName("__InvalidInternalRegionName", ira);
+} catch (IllegalArgumentException ignore) {
+}
+  }
+
+  @Test
+  public void testInternalRegionNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(true);
+LocalRegion.validateRegionName("__ValidInternalRegionName", ira);
+  }
+
+  private void validateCharacters(InternalRegionArguments ira) {
+for (int x = 0; x < Character.MAX_VALUE; x++) {
+  String name = (char) x + REGION_NAME;
+  Matcher matcher = NAME_PATTERN.matcher(name);
+  if (matcher.matches()) {
+LocalRegion.validateRegionName(name, ira);
+  } else {
+try {
+  LocalRegion.validateRegionName(name, ira);
+  fail("Should have received an IllegalArgumentException");
--- End diff --

I added printing the offending character and the numeric value of it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode pull request #285: GEODE-1617: Regions can be created with a...

[kjduling]

Github user kjduling commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/285#discussion_r88080988
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/RegionNameValidationJUnitTest.java
 ---
@@ -0,0 +1,75 @@
+package org.apache.geode.cache;
+
+import static org.junit.Assert.fail;
+
+import org.apache.geode.internal.cache.InternalRegionArguments;
+import org.apache.geode.internal.cache.LocalRegion;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+@Category(UnitTest.class)
+public class RegionNameValidationJUnitTest {
+  private static final Pattern NAME_PATTERN = 
Pattern.compile("[aA-zZ0-9-_.]+");
+  private static final String REGION_NAME = "MyRegion";
+
+
+  @Test
+  public void testInvalidNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+try {
+  LocalRegion.validateRegionName(null, ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+  LocalRegion.validateRegionName("", ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+  LocalRegion.validateRegionName("FOO" + Region.SEPARATOR, ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+
+  }
+
+  @Test
+  public void testExternalRegionNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+validateCharacters(ira);
+try {
+  LocalRegion.validateRegionName("__InvalidInternalRegionName", ira);
--- End diff --

Yes, fixed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode pull request #285: GEODE-1617: Regions can be created with a...

[kirklund]

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/285#discussion_r88075502
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/RegionNameValidationJUnitTest.java
 ---
@@ -0,0 +1,75 @@
+package org.apache.geode.cache;
+
+import static org.junit.Assert.fail;
+
+import org.apache.geode.internal.cache.InternalRegionArguments;
+import org.apache.geode.internal.cache.LocalRegion;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+@Category(UnitTest.class)
+public class RegionNameValidationJUnitTest {
+  private static final Pattern NAME_PATTERN = 
Pattern.compile("[aA-zZ0-9-_.]+");
+  private static final String REGION_NAME = "MyRegion";
+
+
+  @Test
+  public void testInvalidNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+try {
+  LocalRegion.validateRegionName(null, ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+  LocalRegion.validateRegionName("", ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+  LocalRegion.validateRegionName("FOO" + Region.SEPARATOR, ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+
+  }
+
+  @Test
+  public void testExternalRegionNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+validateCharacters(ira);
+try {
+  LocalRegion.validateRegionName("__InvalidInternalRegionName", ira);
+} catch (IllegalArgumentException ignore) {
+}
+  }
+
+  @Test
+  public void testInternalRegionNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(true);
+LocalRegion.validateRegionName("__ValidInternalRegionName", ira);
+  }
+
+  private void validateCharacters(InternalRegionArguments ira) {
+for (int x = 0; x < Character.MAX_VALUE; x++) {
+  String name = (char) x + REGION_NAME;
+  Matcher matcher = NAME_PATTERN.matcher(name);
+  if (matcher.matches()) {
+LocalRegion.validateRegionName(name, ira);
+  } else {
+try {
+  LocalRegion.validateRegionName(name, ira);
+  fail("Should have received an IllegalArgumentException");
--- End diff --

Recommend adding which character to the message so we get instant info 
about which character should have been illegal but was allowed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode pull request #285: GEODE-1617: Regions can be created with a...

[kirklund]

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/285#discussion_r88075036
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/RegionNameValidationJUnitTest.java
 ---
@@ -0,0 +1,75 @@
+package org.apache.geode.cache;
+
+import static org.junit.Assert.fail;
+
+import org.apache.geode.internal.cache.InternalRegionArguments;
+import org.apache.geode.internal.cache.LocalRegion;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+@Category(UnitTest.class)
+public class RegionNameValidationJUnitTest {
+  private static final Pattern NAME_PATTERN = 
Pattern.compile("[aA-zZ0-9-_.]+");
+  private static final String REGION_NAME = "MyRegion";
+
+
+  @Test
+  public void testInvalidNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+try {
+  LocalRegion.validateRegionName(null, ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+  LocalRegion.validateRegionName("", ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+  LocalRegion.validateRegionName("FOO" + Region.SEPARATOR, ira);
+  fail();
+} catch (IllegalArgumentException ignore) {
+}
+
+  }
+
+  @Test
+  public void testExternalRegionNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+validateCharacters(ira);
+try {
+  LocalRegion.validateRegionName("__InvalidInternalRegionName", ira);
--- End diff --

Aren't you missing a fail() stmt here?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode issue #283: GEODE-2098: Moved gfsh history file location fro...

[kirklund]

Github user kirklund commented on the issue:

https://github.com/apache/incubator-geode/pull/283
  
+1 looks ready to push to develop. 

Once it's committed on develop, then mark the Jira ticket Resolved (not 
Closed) and Fix Version(s) should say 1.1.0-incubating. When 1.1.0 is released, 
all Resolved tickets are moved to Closed (all such tickets will be part of 
release notes automatically I think).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode pull request #284: Improved cleanup for UITests

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/incubator-geode/pull/284


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode issue #285: GEODE-1617: Regions can be created with a variet...

[kirklund]

Github user kirklund commented on the issue:

https://github.com/apache/incubator-geode/pull/285
  
I reworded your commit message:

```
GEODE-1617: add test for region names

Added comprehensive test to validate region names

This closes #285
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode issue #285: GEODE-1617: Regions can be created with a variet...

[kirklund]

Github user kirklund commented on the issue:

https://github.com/apache/incubator-geode/pull/285
  
+1


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode pull request #285: GEODE-1617: Regions can be created with a...

[kjduling]

GitHub user kjduling opened a pull request:

https://github.com/apache/incubator-geode/pull/285

GEODE-1617: Regions can be created with a variety of characters that …

…are unsupported

Added comprehensive test to validate region names

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/kjduling/incubator-geode feature/GEODE-1617

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-geode/pull/285.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #285


commit 1672018170c979a4104dce83224a3d697becfe13
Author: Kevin Duling 
Date:   2016-11-15T17:33:59Z

GEODE-1617: Regions can be created with a variety of characters that are 
unsupported

Added comprehensive test to validate region names




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode issue #284: Improved cleanup for UITests

[jaredjstewart]

Github user jaredjstewart commented on the issue:

https://github.com/apache/incubator-geode/pull/284
  
Precheckin passed (except for one failure in FlakyTest)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-geode pull request #284: Improved cleanup for UITests

[jaredjstewart]

GitHub user jaredjstewart opened a pull request:

https://github.com/apache/incubator-geode/pull/284

Improved cleanup for UITests

UITests were failing when more than one was run because they all share a 
base class that was not properly cleaned up.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jaredjstewart/incubator-geode 
uiTestsStepOnEachOther

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-geode/pull/284.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #284


commit 778934f90b6d7e12ce1cc22250dd704f1f5e61ca
Author: Jared Stewart 
Date:   2016-11-15T00:06:49Z

Improved cleanup for UITests

UITests were failing when more than one was run because they all share a 
base class that was not properly cleaned up.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Re: Review Request 53784: GEODE-1247: unable to stop server using http connection

[Kevin Duling]

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



Why would we move away from using Jackson in favor of our own JSON mapping 
code?  Or is this a special case where we do some magic serialization we can't 
do via Jackson?

- Kevin Duling


On Nov. 15, 2016, 7:06 a.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53784/
> ---
> 
> (Updated Nov. 15, 2016, 7:06 a.m.)
> 
> 
> Review request for geode, Jens Deppe, John Blum, Kevin Duling, and Kirk Lund.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-1247: unable to stop server using http connection
> 
> I believe after we added the "com.fasterxml.jackson.databind.ObjectMapper" 
> into core, the a mapper is added to the RestTemplate to precedes our own 
> message converter (SerializableObjectHttpMessageConverter), so on the client 
> side, we are using Jackson's converter to convert the message, but on the 
> server side, we are using our own converter, this caused the message corrupt 
> error. Fix is to remove the Jackson's converter from the client side.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/util/IOUtils.java 
> 4c1cc5f79a3bed922347835441f6624931bdcc6b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java
>  25eee8295ac553da416eb928b160fe7897b17763 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53784/diff/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 53685: GEODE-1955: properly disconnect gfsh session so that it won't leave heartbeat thread around to pollute other tests

[Kevin Duling]

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


Ship it!




Ship It!

- Kevin Duling


On Nov. 14, 2016, 12:56 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53685/
> ---
> 
> (Updated Nov. 14, 2016, 12:56 p.m.)
> 
> 
> Review request for geode, Kevin Duling, Kirk Lund, and Udo Kohlmeyer.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * add the "disconnect" command when we close the GfshShellConnectionRule so 
> that no heartbeat thread is left to pollute other tests.
> * Fix the test so that it truely test the jmx ssl connection, and fix the 
> configuration mishaps.
> * The above fix revealed another bug (GEODE-2099: race condition), ignored 
> the other two tests for now.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java
>  76fb04169f06f82022858cbd0a03eadac8a42ef6 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/JmxManagerAdvisee.java
>  0467c486a0e343bf745fe743172ed4fce750c5b4 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java
>  456f76bb5592b000c75fe733553219c09d1b5381 
>   
> geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java
>  b6afcca7cf8a27c2ad90d6ea570755a63ac0e89b 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
>  44da08be416240f49cbe9dfb8653f41eaea8777e 
> 
> Diff: https://reviews.apache.org/r/53685/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Build failed in Jenkins: Geode-nightly #654

[Apache Jenkins Server]

See 

Changes:

[bschuchardt] GEODE-2074

[bschuchardt] reformatted recent commit with spotlessApply

[klund] GEODE-2102: annotate flaky test with FlakyTest category

[kmiller] GEODE-2105 Remove docs about log collection utility

[kmiller] GEODE-2101 Improve WAN topology terminology in docs

--
[...truncated 565 lines...]
Note: Recompile with -Xlint:unchecked for details.

:geode-lucene:processTestResources
:geode-lucene:testClasses
:geode-lucene:checkMissedTests
:geode-lucene:spotlessJavaCheck
:geode-lucene:spotlessCheck
:geode-lucene:test
:geode-lucene:check
:geode-lucene:build
:geode-lucene:distributedTest
:geode-lucene:flakyTest
:geode-lucene:integrationTest
:geode-old-client-support:assemble
:geode-old-client-support:compileTestJava
:geode-old-client-support:processTestResources UP-TO-DATE
:geode-old-client-support:testClasses
:geode-old-client-support:checkMissedTests
:geode-old-client-support:spotlessJavaCheck
:geode-old-client-support:spotlessCheck
:geode-old-client-support:test
:geode-old-client-support:check
:geode-old-client-support:build
:geode-old-client-support:distributedTest
:geode-old-client-support:flakyTest
:geode-old-client-support:integrationTest
:geode-pulse:assemble
:geode-pulse:compileTestJavaNote: 

 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

:geode-pulse:processTestResources
:geode-pulse:testClasses
:geode-pulse:checkMissedTests
:geode-pulse:spotlessJavaCheck
:geode-pulse:spotlessCheck
:geode-pulse:test
:geode-pulse:check
:geode-pulse:build
:geode-pulse:distributedTest
:geode-pulse:flakyTest
:geode-pulse:integrationTest
:geode-rebalancer:assemble
:geode-rebalancer:compileTestJava
:geode-rebalancer:processTestResources UP-TO-DATE
:geode-rebalancer:testClasses
:geode-rebalancer:checkMissedTests
:geode-rebalancer:spotlessJavaCheck
:geode-rebalancer:spotlessCheck
:geode-rebalancer:test
:geode-rebalancer:check
:geode-rebalancer:build
:geode-rebalancer:distributedTest
:geode-rebalancer:flakyTest
:geode-rebalancer:integrationTest
:geode-wan:assemble
:geode-wan:compileTestJavaNote: Some input files use or override a deprecated 
API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

:geode-wan:processTestResources
:geode-wan:testClasses
:geode-wan:checkMissedTests
:geode-wan:spotlessJavaCheck
:geode-wan:spotlessCheck
:geode-wan:test
:geode-wan:check
:geode-wan:build
:geode-wan:distributedTest
:geode-wan:flakyTest
:geode-wan:integrationTest
:geode-web:assemble
:geode-web:compileTestJavaNote: Some input files use or override a deprecated 
API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

:geode-web:processTestResources UP-TO-DATE
:geode-web:testClasses
:geode-web:checkMissedTests
:geode-web:spotlessJavaCheck
:geode-web:spotlessCheck
:geode-web:test
:geode-web:check
:geode-web:build
:geode-web:distributedTest
:geode-web:flakyTest
:geode-web:integrationTest
:geode-web-api:assemble
:geode-web-api:compileTestJava UP-TO-DATE
:geode-web-api:processTestResources UP-TO-DATE
:geode-web-api:testClasses UP-TO-DATE
:geode-web-api:checkMissedTests UP-TO-DATE
:geode-web-api:spotlessJavaCheck
:geode-web-api:spotlessCheck
:geode-web-api:test UP-TO-DATE
:geode-web-api:check
:geode-web-api:build
:geode-web-api:distributedTest UP-TO-DATE
:geode-web-api:flakyTest UP-TO-DATE
:geode-web-api:integrationTest UP-TO-DATE
:combineReports
All test reports at 

:extensions/geode-modules:precheckin
:extensions/geode-modules-assembly:precheckin
:extensions/geode-modules-session:precheckin
:extensions/geode-modules-session-internal:precheckin
:extensions/geode-modules-tomcat7:precheckin
:extensions/geode-modules-tomcat8:precheckin
:buildExamples
:geode-examples:replicated:compileJavaNote: 

 uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

:geode-examples:replicated:processResources UP-TO-DATE
:geode-examples:replicated:classes
:geode-examples:replicated:jar
:geode-examples:replicated:assemble
:geode-examples:utils:compileJavaNote: 

 uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

Review Request 53784: GEODE-1247: unable to stop server using http connection

[Jinmei Liao]

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

Review request for geode, Jens Deppe, John Blum, Kevin Duling, and Kirk Lund.


Repository: geode


Description
---

GEODE-1247: unable to stop server using http connection

I believe after we added the "com.fasterxml.jackson.databind.ObjectMapper" into 
core, the a mapper is added to the RestTemplate to precedes our own message 
converter (SerializableObjectHttpMessageConverter), so on the client side, we 
are using Jackson's converter to convert the message, but on the server side, 
we are using our own converter, this caused the message corrupt error. Fix is 
to remove the Jackson's converter from the client side.


Diffs
-

  geode-core/src/main/java/org/apache/geode/internal/util/IOUtils.java 
4c1cc5f79a3bed922347835441f6624931bdcc6b 
  
geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java
 25eee8295ac553da416eb928b160fe7897b17763 
  
geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/53784/diff/


Testing
---

precheckin running


Thanks,

Jinmei Liao