date:20170803

[GitHub] geode pull request #664: Fix for GEODE-3292

2017-08-03 Thread jujoramos

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

https://github.com/apache/geode/pull/664#discussion_r131071353
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java
 ---
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.tools.pulse;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
--- End diff --

Yup, you're right, I didn't see the security manager within the rule and 
just looked at the method implementation. The login test doesn't seem to be 
functionality related to the connectivity test, that's why I removed it 
completely and aggregated the tests in a single class with two inner classes.


---
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] geode pull request #664: Fix for GEODE-3292

2017-08-03 Thread jujoramos

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

https://github.com/apache/geode/pull/664#discussion_r131071365
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java
 ---
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.tools.pulse;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
+@Category(IntegrationTest.class)
+public class PulseConnectivityTest {
+  @ClassRule
+  public static LocatorStarterRule locator = new 
LocatorStarterRule().withJMXManager();
+
+  // Test Connectivity for Default Configuration
+  @Category(IntegrationTest.class)
+  public static class DefaultBindAddressTest {
+@Rule
+public EmbeddedPulseRule pulse = new EmbeddedPulseRule();
+
+@BeforeClass
+public static void beforeClass() throws Exception {
--- End diff --

True, but I'm not using `withAutoStart()` for a good reason: the two inner 
classes need to setup the locator differently (the `jmx-manager-bind-address` 
value changes). 
I thought it's easier to read a single functional test class annotated with 
`@RunWith(Enclosed.class)` internally split in different inner classes than 
write a new test class to test exactly the same but changing a single locator 
property.



---
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] geode pull request #664: Fix for GEODE-3292

2017-08-03 Thread jujoramos

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

https://github.com/apache/geode/pull/664#discussion_r131071385
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
 ---
@@ -270,6 +271,7 @@ private void startHttpService(boolean isServer) {
   }
 
   System.setProperty(PULSE_EMBEDDED_PROP, "true");
+  System.setProperty(PULSE_HOST_PROP, "" + 
config.getJmxManagerBindAddress());
--- End diff --

I'll rewrite the tests and push the changes again, thanks for the 
suggestions!.


---
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: [DISCUSS] Feature branch cleanup

2017-08-03 Thread Ernest Burghardt

+1 to using your own fork for feature work

On Wed, Aug 2, 2017 at 10:58 PM, Jacob Barrett  wrote:

> Along those same lines it would be really nice if people would use their
> own forks for feature branches, then we wouldn't have this mess at all.
>
> Sent from my iPhone
>
> > On Aug 2, 2017, at 9:44 PM, Nabarun Nag  wrote:
> >
> >
> > Hi
> > I was wondering if we should clean up the feature branches related to
> closed or resolved GEODE tickets in the origin repository. Below are the
> listed tickets and associated branch.
> >
> > Regards
> > Nabarun Nag
> >
> >
> > Branch Name
> > Ticket
> > Status
> > Committers [merged to develop]
> > origin/feature/GEODE-11
> > GEODE-11
> > Closed
> > [Ashvin Agrawal, [~adharmakkan], [~agingade], [~barry.oglesby],
> [~huynhja], [~upthewaterspout], zhouxh]
> >  origin/feature/GEODE-1209
> > GEODE-1209
> > Resolved
> > [[~agingade], [~dbarnes97]]
> >  origin/feature/GEODE-1912
> > GEODE-1912
> > Resolved
> > [[~jinmeiliao]]
> >  origin/feature/GEODE-1969
> > GEODE-1969
> > Closed
> > [shankar]
> >  origin/feature/GEODE-1987
> > GEODE-1987
> > Resolved
> > []
> >  origin/feature/GEODE-1996
> > GEODE-1996
> > Closed
> > [[~dbarnes97]]
> >  origin/feature/GEODE-2097
> > GEODE-2097
> > Closed
> > [[~dschneider]]
> >  origin/feature/GEODE-2201
> > GEODE-2201
> > Closed
> > [[~jens.deppe]]
> >  origin/feature/GEODE-2231
> > GEODE-2231
> > Closed
> > [[~karensmolermiller]]
> >  origin/feature/GEODE-2367
> > GEODE-2367
> > Closed
> > [[~huynhja]]
> >  origin/feature/GEODE-2398
> > GEODE-2398
> > Closed
> > [[~lgallinat]]
> >  origin/feature/GEODE-2398overflow
> > GEODE-2398
> > Closed
> > [[~lgallinat]]
> >  origin/feature/GEODE-2402
> > GEODE-2402
> > Closed
> > [[~upthewaterspout]]
> >  origin/feature/GEODE-2420
> > GEODE-2420
> > Closed
> > [[~dbarnes97], [~khowe]]
> >  origin/feature/GEODE-2485
> > GEODE-2485
> > Closed
> > [[~dschneider]]
> >  origin/feature/GEODE-2497
> > GEODE-2497
> > Closed
> > [[~bschuchardt]]
> >  origin/feature/GEODE-2541
> > GEODE-2541
> > Closed
> > [[~khowe]]
> >  origin/feature/GEODE-2558
> > GEODE-2558
> > Resolved
> > [[~apa...@the9muses.net ], [~jstewart]]
> >  origin/feature/GEODE-2589
> > GEODE-2589
> > Closed
> > [[~lhughesgodfrey]]
> >  origin/feature/GEODE-2594
> > GEODE-2594
> > Resolved
> > [[~apa...@the9muses.net ],
> [~karensmolermiller]]
> >  origin/feature/GEODE-2599
> > GEODE-2599
> > Closed
> > [[~khowe]]
> >  origin/feature/GEODE-2616
> > GEODE-2616
> > Closed
> > [[~dschneider]]
> >  origin/feature/GEODE-2632
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-1
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-10
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-11
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-12
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-13
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-14
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-15
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-3
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-4
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-5
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-6
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-6-1
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-7
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2632-8
> > GEODE-2632
> > Resolved
> > [[~apa...@the9muses.net ], [~jinmeiliao]]
> >  origin/feature/GEODE-2645
> > GEODE-2645
> > Closed
> > [[~apa...@the9muses.net ]]
> >  origin/feature/GEODE-2648
> > GEODE-2648
> > Closed
> > [[~apa...@the9muses.net ]]
> >  origin/feature/GEODE-2654
> > GEODE-2654
> > Resolved
> > [[~lgallinat]]
> >  origin/feature/GEODE-2661
> > GEODE-2661
> > Closed
> > [[~lgallinat]]
> >

[GitHub] geode issue #677: GEODE-3038: A server process shuts down quietly when path ...

2017-08-03 Thread anton-mironenko

Github user anton-mironenko commented on the issue:

https://github.com/apache/geode/pull/677
  
@dschneider-pivotal Thank you for your feedback. I've replaced 
CacheXmlException with RuntimeException. Sorry for two duplicate commits 
instead of the only one.


---
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] geode pull request #677: GEODE-3038: A server process shuts down quietly whe...

2017-08-03 Thread anton-mironenko

Github user anton-mironenko commented on a diff in the pull request:

https://github.com/apache/geode/pull/677#discussion_r131154689
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 
---
@@ -1208,6 +1208,9 @@ private void initialize() {
   this.system.getConfig());
   initializeDeclarativeCache();
   completedCacheXml = true;
+} catch (CacheXmlException e) {
--- End diff --

@dschneider-pivotal Thank you for your feedback. I've replaced 
CacheXmlException with RuntimeException. Sorry for two duplicate commits 
instead of the only one.


---
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] geode pull request #664: Fix for GEODE-3292

2017-08-03 Thread jinmeiliao

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

https://github.com/apache/geode/pull/664#discussion_r131182711
  
--- Diff: 
geode-pulse/src/test/java/org/apache/geode/tools/pulse/internal/PulseAppListenerTest.java
 ---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ *
+ */
+
+package org.apache.geode.tools.pulse.internal;
+
+import javax.servlet.ServletContextEvent;
+import static org.mockito.Mockito.*;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestRule;
+
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.apache.geode.tools.pulse.internal.data.PulseConstants;
+import org.apache.geode.tools.pulse.internal.data.Repository;
+
+@Category(UnitTest.class)
+public class PulseAppListenerTest {
+  private Repository repository;
+  private PulseAppListener appListener;
+
+  @Rule
+  public final TestRule restoreSystemProperties = new 
RestoreSystemProperties();
+
+  @Before
+  public void setUp() {
+repository = Repository.get();
+appListener = new PulseAppListener();
+  }
+
+  @Test
+  public void embeddedModeDefaultPropertiesRepositoryInitializationTest() {
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, 
"true");
+appListener.contextInitialized(mock(ServletContextEvent.class));
+
+Assert.assertEquals(false, repository.getJmxUseLocator());
+Assert.assertEquals(false, repository.isUseSSLManager());
+Assert.assertEquals(false, repository.isUseSSLLocator());
+Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_PORT, 
repository.getPort());
+Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_HOST, 
repository.getHost());
+
+  }
+
+  @Test
+  public void 
embeddedModeNonDefaultPropertiesRepositoryInitializationTest() {
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, 
"true");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_PORT, "");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_HOST, 
"nonDefaultBindAddress");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_MANAGER,
+Boolean.TRUE.toString());
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_LOCATOR,
+Boolean.TRUE.toString());
+
+appListener.contextInitialized(mock(ServletContextEvent.class));
+
+Assert.assertEquals(false, repository.getJmxUseLocator());
+Assert.assertEquals(true, repository.isUseSSLManager());
+Assert.assertEquals(true, repository.isUseSSLLocator());
+Assert.assertEquals("", repository.getPort());
+Assert.assertEquals("nonDefaultBindAddress", repository.getHost());
+  }
+
+  @After
+  public void tearDown() {
+if (repository != null) {
--- End diff --

this looks familiar, can you use EmbeddedPulseRule for this?


---
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] geode pull request #664: Fix for GEODE-3292

2017-08-03 Thread jinmeiliao

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

https://github.com/apache/geode/pull/664#discussion_r131182576
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/AbstractPulseConnectivityTest.java
 ---
@@ -15,26 +15,31 @@
 
 package org.apache.geode.tools.pulse;
 
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
 import static org.assertj.core.api.Assertions.assertThat;
 
-import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
-import org.apache.geode.test.dunit.rules.HttpClientRule;
-import org.apache.geode.test.dunit.rules.LocatorStarterRule;
-import org.apache.geode.test.junit.categories.IntegrationTest;
-import org.apache.geode.tools.pulse.internal.data.Cluster;
+import java.io.File;
+import java.net.InetAddress;
+import java.util.Properties;
+
 import org.apache.http.HttpResponse;
 import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
+import org.junit.BeforeClass;
 
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.HttpClientRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
 
-@Category(IntegrationTest.class)
-public class PulseVerificationTest {
+public abstract class AbstractPulseConnectivityTest {
--- End diff --

We would like to get rid of the usage of abstract test classes in favor of 
rules and parameterized tests. I see further improvements on these tests. But 
if you don't mind, I can pull your changes in and make modifications on top of 
yours.


---
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] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...

2017-08-03 Thread darrenfoong

Github user darrenfoong commented on the issue:

https://github.com/apache/geode/pull/668
  
I just realised I'll need to unset the property to prevent any side effects 
in the other tests; working on it now.


---
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: [DISCUSS] Feature branch cleanup

2017-08-03 Thread Anthony Baker

+1 I did this awhile back.  Once you’ve finished work on a feature branch 
please remove it.

https://stackoverflow.com/questions/2003505/how-do-i-delete-a-git-branch-both-locally-and-remotely

Anthony


> On Aug 2, 2017, at 9:54 PM, Nabarun Nag  wrote:
> 
> Branch Name
> Ticket
> Status
> Committers to develop
> origin/feature/GEODE-11
> GEODE-11
> Closed
> [Ashvin Agrawal, [~adharmakkan], [~agingade], [~barry.oglesby], [~huynhja], 
> [~upthewaterspout], zhouxh]
>   origin/feature/GEODE-1209
> GEODE-1209
> Resolved
> [[~agingade], [~dbarnes97]]
>   origin/feature/GEODE-1912
> GEODE-1912
> Resolved
> [[~jinmeiliao]]
>   origin/feature/GEODE-1969
> GEODE-1969
> Closed
> [shankar]
>   origin/feature/GEODE-1987
> GEODE-1987
> Resolved
> []
>   origin/feature/GEODE-1996
> GEODE-1996
> Closed
> [[~dbarnes97]]
>   origin/feature/GEODE-2097
> GEODE-2097
> Closed
> [[~dschneider]]
>   origin/feature/GEODE-2201
> GEODE-2201
> Closed
> [[~jens.deppe]]
>   origin/feature/GEODE-2231
> GEODE-2231
> Closed
> [[~karensmolermiller]]
>   origin/feature/GEODE-2367
> GEODE-2367
> Closed
> [[~huynhja]]
>   origin/feature/GEODE-2398
> GEODE-2398
> Closed
> [[~lgallinat]]
>   origin/feature/GEODE-2398overflow
> GEODE-2398
> Closed
> [[~lgallinat]]
>   origin/feature/GEODE-2402
> GEODE-2402
> Closed
> [[~upthewaterspout]]
>   origin/feature/GEODE-2420
> GEODE-2420
> Closed
> [[~dbarnes97], [~khowe]]
>   origin/feature/GEODE-2485
> GEODE-2485
> Closed
> [[~dschneider]]
>   origin/feature/GEODE-2497
> GEODE-2497
> Closed
> [[~bschuchardt]]
>   origin/feature/GEODE-2541
> GEODE-2541
> Closed
> [[~khowe]]
>   origin/feature/GEODE-2558
> GEODE-2558
> Resolved
> [[~apa...@the9muses.net ], [~jstewart]]
>   origin/feature/GEODE-2589
> GEODE-2589
> Closed
> [[~lhughesgodfrey]]
>   origin/feature/GEODE-2594
> GEODE-2594
> Resolved
> [[~apa...@the9muses.net ], [~karensmolermiller]]
>   origin/feature/GEODE-2599
> GEODE-2599
> Closed
> [[~khowe]]
>   origin/feature/GEODE-2616
> GEODE-2616
> Closed
> [[~dschneider]]
>   origin/feature/GEODE-2632
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-1
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-10
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-11
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-12
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-13
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-14
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-15
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-3
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-4
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-5
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-6
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-6-1
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-7
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-8
> GEODE-2632
> Resolved
> [[~apa...@the9muses.net ], [~jinmeiliao]]
>   origin/feature/GEODE-2645
> GEODE-2645
> Closed
> [[~apa...@the9muses.net ]]
>   origin/feature/GEODE-2648
> GEODE-2648
> Closed
> [[~apa...@the9muses.net ]]
>   origin/feature/GEODE-2654
> GEODE-2654
> Resolved
> [[~lgallinat]]
>   origin/feature/GEODE-2661
> GEODE-2661
> Closed
> [[~lgallinat]]
>   origin/feature/GEODE-2681
> GEODE-2681
> Resolved
> [[~khowe]]
>   origin/feature/GEODE-2703
> GEODE-2703
> Closed
> [[~huynhja]]
>   origin/feature/GEODE-2737
> GEODE-2737
> Closed
> [[~khowe]]
>   origin/feature/GEODE-2745
> GEODE-2745
> Closed
> [[~lhughesgodfrey]]
>   origin/feature/GEODE-2765
> GEODE-2765
> Closed
> [[~apa...@the9muses.net ]]
>   origin/feature/GEODE-2768
> GEODE-2768
> Closed
> [[~huynhja]]
>   origin/feature/GEODE-2801
> GEODE-2801
> Closed
> [[~dschneider]]
>   origin/feature/GEODE-2804
> GEODE-2804
> Resolved
> [[~hitesh.khamesra]]
>   origin/feature/GEODE-28

[GitHub] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...

2017-08-03 Thread darrenfoong

Github user darrenfoong commented on the issue:

https://github.com/apache/geode/pull/668
  
I've added code to unset/clear the temporarily-set system properties for 
testing.

Thank you Kenneth for your feedback.


---
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: behavior of "connect" command when --use-ssl

2017-08-03 Thread Kirk Lund

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 permissions to read gemfire.properties can
be blocked from viewing the contents of gfSecurity.properties. I don't know
if this is useful or still a requirement for existing Users or not.

I think another purpose for gfSecurity.properties was to avoid having
system properties such as -Dmy.password=foo from showing up in the list of
Java processes when using a command such as ps. There must be a better way
to protect against exposing sensitive configuration values.

On Wed, Aug 2, 2017 at 10:15 PM, Jinmei Liao  wrote:

> I am looking at a behavior of Gfsh's connect command using ssl. I am not
> sure whether it's a valid use case or just some side effect of spaghetti
> code.
>
> So if SSL is configured on locator, and we need to connect to it in Gfsh
> gfsh>connect --security-properties-file=ssl.properties
> this will try to load the file for ssl configs, and use that for
> connection, sounds good.
>
> gfsh> connect --use-ssl
> this will look for a gfSecurity.properties file in current location, home
> dir, or classpath in order and use that for connection. But if that file
> doesn't exist or empty, it will prompt for all the ssl info.
>
> So when user issues "connect --use-ssl", sometimes they will get prompted,
> sometimes not depending on whether this "special" file exists somewhere in
> your environment. This just does not feel right. I am wondering if looking
> for this "special" file really a good feature?
>
> --
> Cheers
>
> Jinmei
>

Re: Review Request 61281: GEODE-3379 Geode transaction may commit on a secondary bucket after bucket rebalance

2017-08-03 Thread anilkumar gingade


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


Fix it, then Ship it!




Ship It!


geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java
Lines 673 (patched)


For distTx we skip for taking locks on secondary.


- anilkumar gingade


On Aug. 2, 2017, 9:12 p.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61281/
> ---
> 
> (Updated Aug. 2, 2017, 9:12 p.m.)
> 
> 
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, 
> and Nick Reich.
> 
> 
> Bugs: GEODE-3379
> https://issues.apache.org/jira/browse/GEODE-3379
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Current geode commit method only hold primary bucket lock when the txRegion 
> is a primary bucket. However, in between the tx operation and commit, a 
> rebalance could cause primary bucket to move. In this case, commit should 
> detect the primary bucket has been moved and throw 
> TransactionDataRebalancedException.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java 
> 2c8c28b 
>   geode-core/src/test/java/org/apache/geode/disttx/PRDistTXDUnitTest.java 
> 77bf740 
>   
> geode-core/src/test/java/org/apache/geode/disttx/PRDistTXWithVersionsDUnitTest.java
>  9ab5145 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
>  6578baa 
> 
> 
> Diff: https://reviews.apache.org/r/61281/diff/2/
> 
> 
> Testing
> ---
> 
> Ongoing precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>

Review Request 61409: GEODE-3328: simplify GfshParserRule

2017-08-03 Thread Jinmei Liao


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

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


Thanks,

Jinmei Liao

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

2017-08-03 Thread Jinmei Liao


---
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 (updated)
---

precheckin running, tests


Thanks,

Jinmei Liao

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

2017-08-03 Thread Emily Yeh


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


Ship it!




Looks good! +1


geode-junit/src/main/java/org/apache/geode/test/compiler/ClassNameExtractor.java
Lines 22 (patched)


Always really impressed by your regexes!



geode-junit/src/main/java/org/apache/geode/test/compiler/JavaCompiler.java
Lines 108 (patched)


Super nitpick: there's just one extra line here that could be deleted.


- Emily Yeh


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 61185: GEODE-3231: use tempWorkingFolder to avoid test log file contamination between tests.

2017-08-03 Thread Emily Yeh


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


Ship it!




Ship It!

- Emily Yeh


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

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

2017-08-03 Thread Kirk Lund


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


Ship it!




Ship It!

- Kirk Lund


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

Review Request 61411: GEODE-3286 Failing to cleanup connections from ConnectionTable receiver table (corrected "stopped" check in previous fix)

2017-08-03 Thread Hitesh Khamesra


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

Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
O'Sullivan, Udo Kohlmeyer, and Brian Rowe.


Repository: geode


Description
---

In previous fix, we were checking "if thread is stopped then add connection to 
receiver list". Instead, it should be if thread is stopped then we should not 
add connection to receiver list. As a fix, we add connection to reciver list if 
connection is not closed or thread is not stopped.

Added unit for it.


Diffs
-

  geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java 
c3ad596 
  geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java 
69fb7a2 
  
geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java 
312c64d 


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


Testing
---


Thanks,

Hitesh Khamesra

Re: [DISCUSS] Feature branch cleanup

2017-08-03 Thread Jinmei Liao

I am not sure why I have so many branches there. I don't remember I created
them. But this is what I get when I tried to delete one of them:

 *$* git fetch origin

 *$* git push origin --delete feature/GEODE-2632-6

error: unable to delete 'feature/GEODE-2632-6': remote ref does not exist

error: failed to push some refs to '
https://git-wip-us.apache.org/repos/asf/geode.git'

On Wed, Aug 2, 2017 at 9:54 PM, Nabarun Nag  wrote:

> Branch Name
> Ticket
> Status
> Committers to develop
> origin/feature/GEODE-11
> GEODE-11
> Closed
> [Ashvin Agrawal, [~adharmakkan], [~agingade], [~barry.oglesby],
> [~huynhja], [~upthewaterspout], zhouxh]
>   origin/feature/GEODE-1209
> GEODE-1209
> Resolved
> [[~agingade], [~dbarnes97]]
>   origin/feature/GEODE-1912
> GEODE-1912
> Resolved
> [[~jinmeiliao]]
>   origin/feature/GEODE-1969
> GEODE-1969
> Closed
> [shankar]
>   origin/feature/GEODE-1987
> GEODE-1987
> Resolved
> []
>   origin/feature/GEODE-1996
> GEODE-1996
> Closed
> [[~dbarnes97]]
>   origin/feature/GEODE-2097
> GEODE-2097
> Closed
> [[~dschneider]]
>   origin/feature/GEODE-2201
> GEODE-2201
> Closed
> [[~jens.deppe]]
>   origin/feature/GEODE-2231
> GEODE-2231
> Closed
> [[~karensmolermiller]]
>   origin/feature/GEODE-2367
> GEODE-2367
> Closed
> [[~huynhja]]
>   origin/feature/GEODE-2398
> GEODE-2398
> Closed
> [[~lgallinat]]
>   origin/feature/GEODE-2398overflow
> GEODE-2398
> Closed
> [[~lgallinat]]
>   origin/feature/GEODE-2402
> GEODE-2402
> Closed
> [[~upthewaterspout]]
>   origin/feature/GEODE-2420
> GEODE-2420
> Closed
> [[~dbarnes97], [~khowe]]
>   origin/feature/GEODE-2485
> GEODE-2485
> Closed
> [[~dschneider]]
>   origin/feature/GEODE-2497
> GEODE-2497
> Closed
> [[~bschuchardt]]
>   origin/feature/GEODE-2541
> GEODE-2541
> Closed
> [[~khowe]]
>   origin/feature/GEODE-2558
> GEODE-2558
> Resolved
> [[~*apa...@the9muses.net* ], [~jstewart]]
>   origin/feature/GEODE-2589
> GEODE-2589
> Closed
> [[~lhughesgodfrey]]
>   origin/feature/GEODE-2594
> GEODE-2594
> Resolved
> [[~*apa...@the9muses.net* ], [~karensmolermiller]]
>   origin/feature/GEODE-2599
> GEODE-2599
> Closed
> [[~khowe]]
>   origin/feature/GEODE-2616
> GEODE-2616
> Closed
> [[~dschneider]]
>   origin/feature/GEODE-2632
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-1
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-10
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-11
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-12
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-13
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-14
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-15
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-3
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-4
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-5
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-6
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-6-1
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-7
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2632-8
> GEODE-2632
> Resolved
> [[~*apa...@the9muses.net* ], [~jinmeiliao]]
>   origin/feature/GEODE-2645
> GEODE-2645
> Closed
> [[~*apa...@the9muses.net* ]]
>   origin/feature/GEODE-2648
> GEODE-2648
> Closed
> [[~*apa...@the9muses.net* ]]
>   origin/feature/GEODE-2654
> GEODE-2654
> Resolved
> [[~lgallinat]]
>   origin/feature/GEODE-2661
> GEODE-2661
> Closed
> [[~lgallinat]]
>   origin/feature/GEODE-2681
> GEODE-2681
> Resolved
> [[~khowe]]
>   origin/feature/GEODE-2703
> GEODE-2703
> Closed
> [[~huynhja]]
>   origin/feature/GEODE-2737
> GEODE-2737
> Closed
> [[~khowe]]
>   origin/feature/GEODE-2745
> GEODE-2745
> Closed
> [[~lhughesgodfrey]]
>   origin/feature/GEODE-2765
> GEODE-2765
> Closed
> [[~*apa...@the9muses.net* ]]
>   origin/feature/GEODE-2768
> GEODE-2768
> Closed
> [[~huynhja]]
>   origin/feature/GEODE-2801
> GEODE-2801
> Closed
> [[~dschneider]]
>   origin/feature/GEODE-2804
> GEODE-2804
> Resolved
> [[~hitesh.khamesra]]
>   origin/feature/GEODE-2804v2
> GEODE-2804
> Resolved
> [[~hitesh.khamesra]]
>   origin/feature/GEODE-2804v3
> GEODE-2804
> Resolved
> [[~hitesh.khamesra]]
>   origin/feature/GEODE-2811
> GEODE-2811
> Closed
> [[~dschneider]]
>   origin/feature/GEODE-2825
> GEODE-2825
> Closed
> [[~huynhja]]
>   origin/feature/GEODE-2852
> GEODE-2852
> Closed
> [[~lhughesgodfrey]]
>   origin/feature/GEO

Re: [DISCUSS] Feature branch cleanup

2017-08-03 Thread Dan Smith

On Thu, Aug 3, 2017 at 1:41 PM, Jinmei Liao  wrote:

> I am not sure why I have so many branches there. I don't remember I created
> them. But this is what I get when I tried to delete one of them:
>
>  *$* git fetch origin
>
>  *$* git push origin --delete feature/GEODE-2632-6
>
> error: unable to delete 'feature/GEODE-2632-6': remote ref does not exist
>

Feature branches that have been deleted on the server don't actually get
removed from your local checkout unit you do a git remote prune. Naba, is
it possible some of these branches were already deleted? Maybe you need to
do a git remote prune origin.

+1 to cleaning these up and +1 to creating feature branches in forks if the
branches are just for review/PRs. Thanks for taking this on Naba!

-Dan

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

2017-08-03 Thread Jinmei Liao


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

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: behavior of "connect" command when --use-ssl

2017-08-03 Thread Jinmei Liao

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 permissions to read gemfire.properties can
> be blocked from viewing the contents of gfSecurity.properties. I don't know
> if this is useful or still a requirement for existing Users or not.
>
> I think another purpose for gfSecurity.properties was to avoid having
> system properties such as -Dmy.password=foo from showing up in the list of
> Java processes when using a command such as ps. There must be a better way
> to protect against exposing sensitive configuration values.
>
> On Wed, Aug 2, 2017 at 10:15 PM, Jinmei Liao  wrote:
>
> > I am looking at a behavior of Gfsh's connect command using ssl. I am not
> > sure whether it's a valid use case or just some side effect of spaghetti
> > code.
> >
> > So if SSL is configured on locator, and we need to connect to it in Gfsh
> > gfsh>connect --security-properties-file=ssl.properties
> > this will try to load the file for ssl configs, and use that for
> > connection, sounds good.
> >
> > gfsh> connect --use-ssl
> > this will look for a gfSecurity.properties file in current location, home
> > dir, or classpath in order and use that for connection. But if that file
> > doesn't exist or empty, it will prompt for all the ssl info.
> >
> > So when user issues "connect --use-ssl", sometimes they will get
> prompted,
> > sometimes not depending on whether this "special" file exists somewhere
> in
> > your environment. This just does not feel right. I am wondering if
> looking
> > for this "special" file really a good feature?
> >
> > --
> > Cheers
> >
> > Jinmei
> >
>



-- 
Cheers

Jinmei

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

2017-08-03 Thread John Blum

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 permissions to read gemfire.properties
> can
> > be blocked from viewing the contents of gfSecurity.properties. I don't
> know
> > if this is useful or still a requirement for existing Users or not.
> >
> > I think another purpose for gfSecurity.properties was to avoid having
> > system properties such as -Dmy.password=foo from showing up in the list
> of
> > Java processes when using a command such as ps. There must be a better
> way
> > to protect against exposing sensitive configuration values.
> >
> > On Wed, Aug 2, 2017 at 10:15 PM, Jinmei Liao  wrote:
> >
> > > I am looking at a behavior of Gfsh's connect command using ssl. I am
> not
> > > sure whether it's a valid use case or just some side effect of
> spaghetti
> > > code.
> > >
> > > So if SSL is configured on locator, and we need to connect to it in
> Gfsh
> > > gfsh>connect --security-properties-file=ssl.properties
> > > this will try to load the file for ssl configs, and use that for
> > > connection, sounds good.
> > >
> > > gfsh> connect --use-ssl
> > > this will look for a gfSecurity.properties file in current location,
> home
> > > dir, or classpath in order and use that for connection. But if that
> file
> > > doesn't exist or empty, it will prompt for all the ssl info.
> > >
> > > So when user issues "connect --use-ssl", sometimes they will get
> > prompted,
> > > sometimes not depending on whether this "special" file exists somewhere
> > in
> > > your environment. This just does not feel right. I am w

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

2017-08-03 Thread Udo Kohlmeyer


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


Ship it!




Ship It!

- Udo Kohlmeyer


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

Review Request 61420: GEODE-3307 CI failure: Uncaught exception in thread Thread[Geode Membership View Creator

2017-08-03 Thread Hitesh Khamesra


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

Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
O'Sullivan, Udo Kohlmeyer, and Brian Rowe.


Repository: geode


Description
---

Now we catch DistributedSystemDisconnectedException in ViewCreatoe thread and 
then mark shutdown true


Diffs
-

  
geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
 40a4254 


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


Testing
---


Thanks,

Hitesh Khamesra

[GitHub] geode pull request #682: GEODE-3393: One-way SSL authentication fails with F...

2017-08-03 Thread kohlmu-pivotal

GitHub user kohlmu-pivotal opened a pull request:

https://github.com/apache/geode/pull/682

GEODE-3393: One-way SSL authentication fails with FileNotFoundException for 
$USER_HOME/.keystore

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [] Is there a JIRA ticket associated with this PR? Is it referenced in 
the commit message?

- [] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?

- [] Is your initial contribution a single, squashed commit?

- [] Does `gradlew build` run cleanly?

- [] Have you written or updated unit tests to verify your changes?

- [] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and
submit an update to your PR as soon as possible. If you need help, please 
send an
email to dev@geode.apache.org.


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

$ git pull https://github.com/apache/geode feature/GEODE-3393

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

https://github.com/apache/geode/pull/682.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 #682


commit 4f5262fa91e715efb5400507a19fd683a7078bf4
Author: Udo Kohlmeyer 
Date:   2017-08-03T21:13:06Z

GEODE-3393: One-way SSL commit failing with userHome/.keystore not found

commit 9a8700af71d14b22caefc026aab6bd01c99590ab
Author: Udo Kohlmeyer 
Date:   2017-08-03T22:09:43Z

GEODE-3393: One-way SSL commit failing with userHome/.keystore not found




---
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] geode issue #682: GEODE-3393: One-way SSL authentication fails with FileNotF...

2017-08-03 Thread kohlmu-pivotal

Github user kohlmu-pivotal commented on the issue:

https://github.com/apache/geode/pull/682
  
@hiteshk25 @bschuchardt @pivotal-amurmann @WireBaron 


---
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: behavior of "connect" command when --use-ssl

2017-08-03 Thread John Blum

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 permissions to read gemfire.properties
> > can
> > > be blocked from viewing the contents of gfSecurity.properties. I don't
> > know
> > > if this is useful or still a requirement for existing Users or not.
> > >
> > > I think another purpose for gfSecurity.properties was to avoid having
> > > system properties such as -Dmy.password=foo from showing up in the list
> > of
> > > Java processes when using a command such as ps. There must be a better
> > way
> > > to protect against exposing sensitive configuration values.
> > >
> > > On Wed, Aug 2, 2017 at 10:15 PM, Jinmei Liao wrote:
> > >
> > > > I am

Re: Review Request 61411: GEODE-3286 Failing to cleanup connections from ConnectionTable receiver table (corrected "stopped" check in previous fix)

2017-08-03 Thread Brian Rowe


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


Fix it, then Ship it!




Nitpicks below, but change looks good.


geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java
Line 36 (original), 40 (patched)


Maybe make this @Before instead of calling it at the start of every test.



geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java
Line 54 (original), 70 (patched)


Add a when(connection.isReceiverStopped()).thenReturn(false);


- Brian Rowe


On Aug. 3, 2017, 6:59 p.m., Hitesh Khamesra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61411/
> ---
> 
> (Updated Aug. 3, 2017, 6:59 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
> O'Sullivan, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> In previous fix, we were checking "if thread is stopped then add connection 
> to receiver list". Instead, it should be if thread is stopped then we should 
> not add connection to receiver list. As a fix, we add connection to reciver 
> list if connection is not closed or thread is not stopped.
> 
> Added unit for it.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java 
> c3ad596 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java 
> 69fb7a2 
>   
> geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java
>  312c64d 
> 
> 
> Diff: https://reviews.apache.org/r/61411/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>

Re: Review Request 61420: GEODE-3307 CI failure: Uncaught exception in thread Thread[Geode Membership View Creator

2017-08-03 Thread Brian Rowe


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


Ship it!




Ship It!

- Brian Rowe


On Aug. 3, 2017, 10:07 p.m., Hitesh Khamesra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61420/
> ---
> 
> (Updated Aug. 3, 2017, 10:07 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
> O'Sullivan, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Now we catch DistributedSystemDisconnectedException in ViewCreatoe thread and 
> then mark shutdown true
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
>  40a4254 
> 
> 
> Diff: https://reviews.apache.org/r/61420/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>

[Spring CI] Spring Data GemFire > Nightly-ApacheGeode > #636 was SUCCESSFUL (with 2024 tests)

2017-08-03 Thread Spring CI


---
Spring Data GemFire > Nightly-ApacheGeode > #636 was successful.
---
Scheduled
2026 tests in total.

https://build.spring.io/browse/SGF-NAG-636/





--
This message is automatically generated by Atlassian Bamboo

Re: Review Request 61411: GEODE-3286 Failing to cleanup connections from ConnectionTable receiver table (corrected "stopped" check in previous fix)

2017-08-03 Thread Hitesh Khamesra


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

(Updated Aug. 3, 2017, 11:01 p.m.)


Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
O'Sullivan, Udo Kohlmeyer, and Brian Rowe.


Changes
---

Updated diff based on review


Repository: geode


Description
---

In previous fix, we were checking "if thread is stopped then add connection to 
receiver list". Instead, it should be if thread is stopped then we should not 
add connection to receiver list. As a fix, we add connection to reciver list if 
connection is not closed or thread is not stopped.

Added unit for it.


Diffs (updated)
-

  geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java 
c3ad596 
  geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java 
69fb7a2 
  
geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java 
312c64d 


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

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


Testing
---


Thanks,

Hitesh Khamesra

[GitHub] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...

2017-08-03 Thread galen-pivotal

GitHub user galen-pivotal opened a pull request:

https://github.com/apache/geode/pull/683

GEODE-3314 - additional refactoring for developer QoL.

* Write characterization tests for DLockService.
* Remove debugging code.
* Remove dead code.
* Remove comments.
* Extract the local lock granting into a separate function.

Between the characterization tests we've written and the existing DUnit
tests, the coverage should be fairly adequate.

Signed-off-by: Hitesh Khamesra 
Signed-off-by: Galen O'Sullivan 

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in 
the commit message?

- [ ] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?

- [x] Is your initial contribution a single, squashed commit?

- [ ] Does `gradlew build` run cleanly?

- [x] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and
submit an update to your PR as soon as possible. If you need help, please 
send an
email to dev@geode.apache.org.


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

$ git pull https://github.com/galen-pivotal/geode feature/GEODE-3314-2

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

https://github.com/apache/geode/pull/683.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 #683


commit 6ec19370da30004347b20c22563268f6adfd35c1
Author: Galen O'Sullivan 
Date:   2017-08-02T18:29:21Z

GEODE-3314 - additional refactoring for developer QoL.

* Write characterization tests for DLockService.
* Remove debugging code.
* Remove dead code.
* Remove comments.
* Extract the local lock granting into a separate function.

Between the characterization tests we've written and the existing DUnit
tests, the coverage should be fairly adequate.

Signed-off-by: Hitesh Khamesra 
Signed-off-by: Galen O'Sullivan 




---
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] geode issue #683: GEODE-3314 - additional refactoring for developer QoL.

2017-08-03 Thread galen-pivotal

Github user galen-pivotal commented on the issue:

https://github.com/apache/geode/pull/683
  
@kohlmu-pivotal @hiteshk25 @bschuchardt @pivotal-amurmann @WireBaron 


---
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 61417: GEODE-3328: adding ssl-truststore-type to the config

2017-08-03 Thread Dave Barnes

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

1. The help message for the property should state what it does and whether it 
has a default. JKS was our assumption in the past, but with the introduction of 
this feature other values (such as 'pkcs12') are possible.
Current wording: "For Java truststore file format, this property has the value 
jks (or JKS)."
Suggested wording: "Specifies the type of the ssl truststore. The default is '' 
(an empty string) For Java truststore format, specify 'jks' or 'JKS'."

The following questions come from me reading for patterns, please forgive if 
they're stupid ones:

2. In configureLegacyClusterSSL (and ..ServerSSL and ..JMXSSL and 
..ServiceSSL), I see two occurrences of 
"sslConfig.setTruststoreType(getDistributionConfig().getClusterSSLKeyStoreType());".
 Should the second one in each case be getClusterSSLTrustStoreType() ?

3. In DistributionConfigImpl.java, there's a long list of static imports for 
gateway, http, jmx, etc. KEYSTORE_TYPE is included for each group, but not 
TRUSTSTORE_TYPE. Is this correct?

- Dave Barnes

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

[GitHub] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...

2017-08-03 Thread WireBaron

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

https://github.com/apache/geode/pull/683#discussion_r131287366
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockRequestProcessor.java
 ---
@@ -196,6 +196,13 @@ long getLeaseExpireTime() {
 return this.response.leaseExpireTime;
   }
 
+  /**
+   *
+   * @param interruptible
+   * @param lockId
+   * @return
+   * @throws InterruptedException only possible if interruptible is true.
+   */
   protected boolean requestLock(boolean interruptible, int lockId) throws 
InterruptedException {
 final boolean isDebugEnabled_DLS = 
logger.isTraceEnabled(LogMarker.DLS);
--- End diff --

Did you want to remove this, as you seem to be removing a lot of switching 
based on it.  Is this code going to be log spammy without this?


---
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] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...

2017-08-03 Thread WireBaron

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

https://github.com/apache/geode/pull/683#discussion_r131287954
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockToken.java
 ---
@@ -302,14 +302,14 @@ boolean checkForExpiration() {
* @param remoteThread identity of the leasing thread
* @return true if lease for this lock token is successfully granted
*/
-  synchronized boolean grantLock(long newLeaseExpireTime, int newLeaseId, 
int newRecursion,
+  synchronized void grantLock(long newLeaseExpireTime, int newLeaseId, int 
newRecursion,
   RemoteThread remoteThread) {
 
 Assert.assertTrue(remoteThread != null);
 Assert.assertTrue(newLeaseId > -1, "Invalid attempt to grant lock with 
leaseId " + newLeaseId);
 
 checkDestroyed();
-checkForExpiration();
+checkForExpiration(); // TODO: this should throw.
--- End diff --

Any chance this TODO can be implemented now?


---
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] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...

2017-08-03 Thread WireBaron

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

https://github.com/apache/geode/pull/683#discussion_r131287871
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockToken.java
 ---
@@ -87,7 +87,8 @@
   private Thread thread;
 
   /**
-   * Number of threads currently using this lock token.
+   * Number of usages of this lock token. usageCount = recursion + (# of 
threads waiting for this
+   * lock). It's weird, I know.
--- End diff --

Did you want the editorial comment pushed?


---
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] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...

2017-08-03 Thread galen-pivotal

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/683#discussion_r131290034
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockToken.java
 ---
@@ -87,7 +87,8 @@
   private Thread thread;
 
   /**
-   * Number of threads currently using this lock token.
+   * Number of usages of this lock token. usageCount = recursion + (# of 
threads waiting for this
+   * lock). It's weird, I know.
--- End diff --

I could go either way. I don't mind a bit of humour and dialogue in our 
codebase.


---
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] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...

2017-08-03 Thread galen-pivotal

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/683#discussion_r131290142
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockRequestProcessor.java
 ---
@@ -196,6 +196,13 @@ long getLeaseExpireTime() {
 return this.response.leaseExpireTime;
   }
 
+  /**
+   *
+   * @param interruptible
+   * @param lockId
+   * @return
+   * @throws InterruptedException only possible if interruptible is true.
+   */
   protected boolean requestLock(boolean interruptible, int lockId) throws 
InterruptedException {
 final boolean isDebugEnabled_DLS = 
logger.isTraceEnabled(LogMarker.DLS);
--- End diff --

The `LogMarker` is used in the `logger.trace` function. This is based on an 
old pattern that was written because an old version of the logger was slow, so 
it was better to gate on a boolean for performance. The logging function will 
do an identical check to this one before printing a log message.


---
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] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...

2017-08-03 Thread galen-pivotal

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/683#discussion_r131290383
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockToken.java
 ---
@@ -302,14 +302,14 @@ boolean checkForExpiration() {
* @param remoteThread identity of the leasing thread
* @return true if lease for this lock token is successfully granted
*/
-  synchronized boolean grantLock(long newLeaseExpireTime, int newLeaseId, 
int newRecursion,
+  synchronized void grantLock(long newLeaseExpireTime, int newLeaseId, int 
newRecursion,
   RemoteThread remoteThread) {
 
 Assert.assertTrue(remoteThread != null);
 Assert.assertTrue(newLeaseId > -1, "Invalid attempt to grant lock with 
leaseId " + newLeaseId);
 
 checkDestroyed();
-checkForExpiration();
+checkForExpiration(); // TODO: this should throw.
--- End diff --

yeah, that's probably a good idea. It looks like `checkForExpiration` is 
being used for the side effect of expiring the lock if it's run out of time. 
This method could be made clearer.


---
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] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...

2017-08-03 Thread galen-pivotal

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/683#discussion_r131290606
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockToken.java
 ---
@@ -302,14 +302,14 @@ boolean checkForExpiration() {
* @param remoteThread identity of the leasing thread
* @return true if lease for this lock token is successfully granted
*/
-  synchronized boolean grantLock(long newLeaseExpireTime, int newLeaseId, 
int newRecursion,
+  synchronized void grantLock(long newLeaseExpireTime, int newLeaseId, int 
newRecursion,
   RemoteThread remoteThread) {
 
 Assert.assertTrue(remoteThread != null);
 Assert.assertTrue(newLeaseId > -1, "Invalid attempt to grant lock with 
leaseId " + newLeaseId);
 
 checkDestroyed();
-checkForExpiration();
+checkForExpiration(); // TODO: this should throw.
--- End diff --

I may have misunderstood the intent of calling this function when I added 
the comment. It could use a second look.


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

43 matches

Mail list logo