[GitHub] [geode] gesterzhou commented on a change in pull request #5492: GEODE-8475: Resolve a potential dead lock in ParallelGatewaySenderQueue

[GitBox]

gesterzhou commented on a change in pull request #5492:
URL: https://github.com/apache/geode/pull/5492#discussion_r481718727



##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##
@@ -755,12 +755,16 @@ public boolean put(Object object) throws 
InterruptedException, CacheException {
 bucketFullPath, brq);
   }
   if (brq != null) {
+boolean intializingLocked = brq.lockWhenRegionIsInitializing();
 brq.getInitializationLock().readLock().lock();
 try {
   putIntoBucketRegionQueue(brq, key, value);
   putDone = true;
 } finally {
   brq.getInitializationLock().readLock().unlock();
+  if (intializingLocked) {

Review comment:
   yes





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] gesterzhou commented on a change in pull request #5492: GEODE-8475: Resolve a potential dead lock in ParallelGatewaySenderQueue

[GitBox]

gesterzhou commented on a change in pull request #5492:
URL: https://github.com/apache/geode/pull/5492#discussion_r481718201



##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##
@@ -755,12 +755,16 @@ public boolean put(Object object) throws 
InterruptedException, CacheException {
 bucketFullPath, brq);
   }
   if (brq != null) {
+boolean intializingLocked = brq.lockWhenRegionIsInitializing();

Review comment:
   added.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-native] moleske commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations

[GitBox]

moleske commented on a change in pull request #645:
URL: https://github.com/apache/geode-native/pull/645#discussion_r481562738



##
File path: tests/cpp/security/XmlAuthzCredentialGenerator.hpp
##
@@ -55,7 +55,7 @@ const opCodeList::value_type QArr[] = {OP_QUERY, 
OP_REGISTER_CQ};
 
 const stringList::value_type QRArr[] = {"Portfolios", "Positions"};
 
-const char* PRiUsnm = "%s%d";
+static const char* PRiUsnm = "%s%d";

Review comment:
   It did not.  `ISO C++11 does not allow conversion from string literal to 
'char *const'` is the specific error when trying to use it while on a mac. 
Haven't looked more into at the moment





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-native] moleske commented on pull request #647: GEODE-8460: Run SNITests Based on Docker Availability

[GitBox]

moleske commented on pull request #647:
URL: https://github.com/apache/geode-native/pull/647#issuecomment-685231238


   >  I think geode-native is sorely lacking in documentation on how to use the 
new test frameworks
   
   For this PR, I think just a one liner mentioning they exist is a good 
enough.  Agree that it could be better in both places



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-native] mmartell commented on pull request #647: GEODE-8460: Run SNITests Based on Docker Availability

[GitBox]

mmartell commented on pull request #647:
URL: https://github.com/apache/geode-native/pull/647#issuecomment-685222727


   > I'm excited about this! It seems like there is a new category of 
`acceptance-test`. There should be an update to CONTRIBUTING.md to make sure 
that contributors run tests in the acceptance test directory
   
   Good catch moleske! In fact I think geode-native is sorely lacking in 
documentation on how to use the new test frameworks to write tests. I'm told 
geode also is lacking in this area.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-native] moleske edited a comment on pull request #647: GEODE-8460: Run SNITests Based on Docker Availability

[GitBox]

moleske edited a comment on pull request #647:
URL: https://github.com/apache/geode-native/pull/647#issuecomment-685198557


   I'm excited about this!  It seems like there is a new category of 
`acceptance-test`.  There should be an update to CONTRIBUTING.md to make sure 
that contributors run tests in the acceptance test directory



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-native] moleske commented on pull request #647: GEODE-8460: Run SNITests Based on Docker Availability

[GitBox]

moleske commented on pull request #647:
URL: https://github.com/apache/geode-native/pull/647#issuecomment-685198557


   I'm excited about this!  It seems like there is a new category of 
`acceptance-test`.  Not sure when difference between integration and acceptance 
is (I think it is just does it need docker), but there should be an update to 
CONTRIBUTING.md to make sure that contributors run tests in the acceptance test 
directory



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] mhansonp commented on a change in pull request #5436: GEODE-7672: add dunit test to verify OQL index after PR clear.

[GitBox]

mhansonp commented on a change in pull request #5436:
URL: https://github.com/apache/geode/pull/5436#discussion_r481498899



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/cache/query/partitioned/PRClearQueryIndexDUnitTest.java
##
@@ -0,0 +1,374 @@
+/*
+ * 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.cache.query.partitioned;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.junit.rules.VMProvider.invokeInEveryMember;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.IntStream;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ServerOperationException;
+import org.apache.geode.cache.query.Index;
+import org.apache.geode.cache.query.IndexStatistics;
+import org.apache.geode.cache.query.Query;
+import org.apache.geode.cache.query.QueryService;
+import org.apache.geode.cache.query.SelectResults;
+import org.apache.geode.cache.query.data.City;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.test.dunit.AsyncInvocation;
+import org.apache.geode.test.dunit.DUnitBlackboard;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.rules.ClientCacheRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class PRClearQueryIndexDUnitTest {
+  public static final String MUMBAI_QUERY = "select * from /cities c where 
c.name = 'MUMBAI'";
+  public static final String ID_10_QUERY = "select * from /cities c where c.id 
= 10";
+  @ClassRule
+  public static ClusterStartupRule cluster = new ClusterStartupRule(4, true);
+
+  private static MemberVM server1;
+  private static MemberVM server2;
+
+  private static DUnitBlackboard blackboard;
+
+  @Rule
+  public ClientCacheRule clientCacheRule = new ClientCacheRule();
+
+  @Rule
+  public ExecutorServiceRule executor = ExecutorServiceRule.builder().build();
+
+  private ClientCache clientCache;
+  private Region cities;
+
+  // class test setup. set up the servers, regions and indexes on the servers
+  @BeforeClass
+  public static void beforeClass() {
+int locatorPort = ClusterStartupRule.getDUnitLocatorPort();
+server1 = cluster.startServerVM(1, s -> 
s.withConnectionToLocator(locatorPort)
+.withProperty(SERIALIZABLE_OBJECT_FILTER, 
"org.apache.geode.cache.query.data.*")
+.withRegion(RegionShortcut.PARTITION, "cities"));
+server2 = cluster.startServerVM(2, s -> 
s.withConnectionToLocator(locatorPort)
+.withProperty(SERIALIZABLE_OBJECT_FILTER, 
"org.apache.geode.cache.query.data.*")
+.withRegion(RegionShortcut.PARTITION, "cities"));
+
+server1.invoke(() -> {
+  Cache cache = ClusterStartupRule.getCache();
+  Region region = cache.getRegion("cities");
+  // create indexes
+  QueryService queryService = cache.getQueryService();
+  queryService.createKeyIndex("cityId", "c.id", "/cities c");
+  queryService.createIndex("cityName", "c.name", "/cities c");
+  assertThat(cache.getQueryService().getIndexes(region))
+  .extracting(Index::getName).containsExactlyInAnyOrder("cityId", 
"cityName");
+});
+
+server2.invoke(() -> {
+  Cache cache = ClusterStartupRule.getCache();
+  Region region = cache.getRegion("cities");
+  assertThat(cache.getQueryService().getIndexes(region))
+  .extracting(Index::getName).containsExactlyInAnyOrder("cityId", 
"cityName");
+});
+  }
+
+  // before every test method, create the client cache and region
+  @Before
+  public void before() throws Exception {
+int locatorPort = 

[GitHub] [geode] mhansonp commented on a change in pull request #5436: GEODE-7672: add dunit test to verify OQL index after PR clear.

[GitBox]

mhansonp commented on a change in pull request #5436:
URL: https://github.com/apache/geode/pull/5436#discussion_r481498183



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/cache/query/partitioned/PRClearQueryIndexDUnitTest.java
##
@@ -0,0 +1,374 @@
+/*
+ * 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.cache.query.partitioned;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.junit.rules.VMProvider.invokeInEveryMember;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.IntStream;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ServerOperationException;
+import org.apache.geode.cache.query.Index;
+import org.apache.geode.cache.query.IndexStatistics;
+import org.apache.geode.cache.query.Query;
+import org.apache.geode.cache.query.QueryService;
+import org.apache.geode.cache.query.SelectResults;
+import org.apache.geode.cache.query.data.City;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.test.dunit.AsyncInvocation;
+import org.apache.geode.test.dunit.DUnitBlackboard;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.rules.ClientCacheRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class PRClearQueryIndexDUnitTest {
+  public static final String MUMBAI_QUERY = "select * from /cities c where 
c.name = 'MUMBAI'";
+  public static final String ID_10_QUERY = "select * from /cities c where c.id 
= 10";
+  @ClassRule
+  public static ClusterStartupRule cluster = new ClusterStartupRule(4, true);
+
+  private static MemberVM server1;
+  private static MemberVM server2;
+
+  private static DUnitBlackboard blackboard;
+
+  @Rule
+  public ClientCacheRule clientCacheRule = new ClientCacheRule();
+
+  @Rule
+  public ExecutorServiceRule executor = ExecutorServiceRule.builder().build();
+
+  private ClientCache clientCache;
+  private Region cities;
+
+  // class test setup. set up the servers, regions and indexes on the servers
+  @BeforeClass
+  public static void beforeClass() {
+int locatorPort = ClusterStartupRule.getDUnitLocatorPort();
+server1 = cluster.startServerVM(1, s -> 
s.withConnectionToLocator(locatorPort)
+.withProperty(SERIALIZABLE_OBJECT_FILTER, 
"org.apache.geode.cache.query.data.*")
+.withRegion(RegionShortcut.PARTITION, "cities"));
+server2 = cluster.startServerVM(2, s -> 
s.withConnectionToLocator(locatorPort)
+.withProperty(SERIALIZABLE_OBJECT_FILTER, 
"org.apache.geode.cache.query.data.*")
+.withRegion(RegionShortcut.PARTITION, "cities"));
+
+server1.invoke(() -> {
+  Cache cache = ClusterStartupRule.getCache();
+  Region region = cache.getRegion("cities");
+  // create indexes
+  QueryService queryService = cache.getQueryService();
+  queryService.createKeyIndex("cityId", "c.id", "/cities c");
+  queryService.createIndex("cityName", "c.name", "/cities c");
+  assertThat(cache.getQueryService().getIndexes(region))
+  .extracting(Index::getName).containsExactlyInAnyOrder("cityId", 
"cityName");
+});
+
+server2.invoke(() -> {
+  Cache cache = ClusterStartupRule.getCache();
+  Region region = cache.getRegion("cities");
+  assertThat(cache.getQueryService().getIndexes(region))
+  .extracting(Index::getName).containsExactlyInAnyOrder("cityId", 
"cityName");
+});
+  }
+
+  // before every test method, create the client cache and region
+  @Before
+  public void before() throws Exception {
+int locatorPort = 

[GitHub] [geode] mhansonp commented on a change in pull request #5436: GEODE-7672: add dunit test to verify OQL index after PR clear.

[GitBox]

mhansonp commented on a change in pull request #5436:
URL: https://github.com/apache/geode/pull/5436#discussion_r481498183



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/cache/query/partitioned/PRClearQueryIndexDUnitTest.java
##
@@ -0,0 +1,374 @@
+/*
+ * 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.cache.query.partitioned;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.junit.rules.VMProvider.invokeInEveryMember;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.IntStream;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ServerOperationException;
+import org.apache.geode.cache.query.Index;
+import org.apache.geode.cache.query.IndexStatistics;
+import org.apache.geode.cache.query.Query;
+import org.apache.geode.cache.query.QueryService;
+import org.apache.geode.cache.query.SelectResults;
+import org.apache.geode.cache.query.data.City;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.test.dunit.AsyncInvocation;
+import org.apache.geode.test.dunit.DUnitBlackboard;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.rules.ClientCacheRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class PRClearQueryIndexDUnitTest {
+  public static final String MUMBAI_QUERY = "select * from /cities c where 
c.name = 'MUMBAI'";
+  public static final String ID_10_QUERY = "select * from /cities c where c.id 
= 10";
+  @ClassRule
+  public static ClusterStartupRule cluster = new ClusterStartupRule(4, true);
+
+  private static MemberVM server1;
+  private static MemberVM server2;
+
+  private static DUnitBlackboard blackboard;
+
+  @Rule
+  public ClientCacheRule clientCacheRule = new ClientCacheRule();
+
+  @Rule
+  public ExecutorServiceRule executor = ExecutorServiceRule.builder().build();
+
+  private ClientCache clientCache;
+  private Region cities;
+
+  // class test setup. set up the servers, regions and indexes on the servers
+  @BeforeClass
+  public static void beforeClass() {
+int locatorPort = ClusterStartupRule.getDUnitLocatorPort();
+server1 = cluster.startServerVM(1, s -> 
s.withConnectionToLocator(locatorPort)
+.withProperty(SERIALIZABLE_OBJECT_FILTER, 
"org.apache.geode.cache.query.data.*")
+.withRegion(RegionShortcut.PARTITION, "cities"));
+server2 = cluster.startServerVM(2, s -> 
s.withConnectionToLocator(locatorPort)
+.withProperty(SERIALIZABLE_OBJECT_FILTER, 
"org.apache.geode.cache.query.data.*")
+.withRegion(RegionShortcut.PARTITION, "cities"));
+
+server1.invoke(() -> {
+  Cache cache = ClusterStartupRule.getCache();
+  Region region = cache.getRegion("cities");
+  // create indexes
+  QueryService queryService = cache.getQueryService();
+  queryService.createKeyIndex("cityId", "c.id", "/cities c");
+  queryService.createIndex("cityName", "c.name", "/cities c");
+  assertThat(cache.getQueryService().getIndexes(region))
+  .extracting(Index::getName).containsExactlyInAnyOrder("cityId", 
"cityName");
+});
+
+server2.invoke(() -> {
+  Cache cache = ClusterStartupRule.getCache();
+  Region region = cache.getRegion("cities");
+  assertThat(cache.getQueryService().getIndexes(region))
+  .extracting(Index::getName).containsExactlyInAnyOrder("cityId", 
"cityName");
+});
+  }
+
+  // before every test method, create the client cache and region
+  @Before
+  public void before() throws Exception {
+int locatorPort = 

[GitHub] [geode] mhansonp commented on pull request #5436: GEODE-7672: add dunit test to verify OQL index after PR clear.

[GitBox]

mhansonp commented on pull request #5436:
URL: https://github.com/apache/geode/pull/5436#issuecomment-685193313


   I made some updates on mhansonp minorcleanup to resolve warnings.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-native] mmartell opened a new pull request #647: GEODE-8460: Run SNITests Based on Docker Availability

[GitBox]

mmartell opened a new pull request #647:
URL: https://github.com/apache/geode-native/pull/647


   This PR cleans up our docker based SNITests. In particular:
   
   - SNITests are now enabled during cmake configuration based on whether 
docker and docker-compose are installed.
   - SNITests have been moved to a new acceptance-test folder to keep them 
separated from tests which don't use docker.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-native] moleske commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations

[GitBox]

moleske commented on a change in pull request #645:
URL: https://github.com/apache/geode-native/pull/645#discussion_r481420359



##
File path: cppcache/integration-test/fw_dunit.cpp
##
@@ -71,10 +71,10 @@ using 
apache::geode::client::testframework::BBNamingContextServer;
 #define __DUNIT_NO_MAIN__
 #include "fw_dunit.hpp"
 
-ACE_TCHAR *g_programName = nullptr;
-uint32_t g_coordinatorPid = 0;
+static ACE_TCHAR *g_programName = nullptr;

Review comment:
   I think the worst part is since I was letting the compiler tell me what 
was wrong rather than proactively look for places, it built only a partial 
percent at a time when it was building the integration-test.  Thanks for the 
reminder on places that should be able to use `const`!  Putting a link to the 
[`const` naming style 
guide](https://google.github.io/styleguide/cppguide#Constant_Names) so I don't 
forget





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-native] moleske commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations

[GitBox]

moleske commented on a change in pull request #645:
URL: https://github.com/apache/geode-native/pull/645#discussion_r481420359



##
File path: cppcache/integration-test/fw_dunit.cpp
##
@@ -71,10 +71,10 @@ using 
apache::geode::client::testframework::BBNamingContextServer;
 #define __DUNIT_NO_MAIN__
 #include "fw_dunit.hpp"
 
-ACE_TCHAR *g_programName = nullptr;
-uint32_t g_coordinatorPid = 0;
+static ACE_TCHAR *g_programName = nullptr;

Review comment:
   I think the worst part is since I was letting the compiler tell me what 
was wrong rather than proactively look for places, I build only a partial 
percent at a time when it was building the integration-test.  Thanks for the 
reminder on places that should be able to use `const`!  Putting a link to the 
[`const` naming style 
guide](https://google.github.io/styleguide/cppguide#Constant_Names) so I don't 
forget





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] boglesby opened a new pull request #5494: GEODE-8478: Refactored logThresholdExceededAlerts and modified it to …

[GitBox]

boglesby opened a new pull request #5494:
URL: https://github.com/apache/geode/pull/5494


   …handle exceptions
   
   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, check Concourse for build 
issues and
   submit an update to your PR as soon as possible. If you need help, please 
send an
   email to d...@geode.apache.org.
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations

[GitBox]

pivotal-jbarrett commented on a change in pull request #645:
URL: https://github.com/apache/geode-native/pull/645#discussion_r481354457



##
File path: cppcache/benchmark/GeodeLoggingBM.cpp
##
@@ -106,13 +106,13 @@ void GeodeLogToFile(benchmark::State& state) {
   }
 }
 
-auto LogStringsToConsole = GeodeLogToConsole;
-auto LogIntsToConsole = GeodeLogToConsole;
-auto LogComboToConsole = GeodeLogToConsole;
+static auto LogStringsToConsole = GeodeLogToConsole;

Review comment:
   `const` also?

##
File path: tests/cpp/security/XmlAuthzCredentialGenerator.hpp
##
@@ -55,7 +55,7 @@ const opCodeList::value_type QArr[] = {OP_QUERY, 
OP_REGISTER_CQ};
 
 const stringList::value_type QRArr[] = {"Portfolios", "Positions"};
 
-const char* PRiUsnm = "%s%d";
+static const char* PRiUsnm = "%s%d";

Review comment:
   `constexpr` work here?

##
File path: cppcache/integration-test/fw_dunit.cpp
##
@@ -71,10 +71,10 @@ using 
apache::geode::client::testframework::BBNamingContextServer;
 #define __DUNIT_NO_MAIN__
 #include "fw_dunit.hpp"
 
-ACE_TCHAR *g_programName = nullptr;
-uint32_t g_coordinatorPid = 0;
+static ACE_TCHAR *g_programName = nullptr;

Review comment:
   Yuck! I attempted to take on this beast and gave up on trying to clean 
up the old tests. Good job!

##
File path: cppcache/test/CacheXmlParserTest.cpp
##
@@ -99,7 +99,7 @@ std::string valid_cache_config_body = R"(
 
 )";
 
-std::string invalid_cache_config_body = R"(
+static std::string invalid_cache_config_body = R"(

Review comment:
   `const` ?

##
File path: cppcache/src/PdxFieldType.cpp
##
@@ -31,7 +31,7 @@ namespace apache {
 namespace geode {
 namespace client {
 
-int32_t fixedTypeSizes[] = {
+static int32_t fixedTypeSizes[] = {

Review comment:
   `const` and use the `const` naming convention here?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] demery-pivotal opened a new pull request #5493: GEODE-8404: Simplify AvailablePortHelper

[GitBox]

demery-pivotal opened a new pull request #5493:
URL: https://github.com/apache/geode/pull/5493


   Removed the following unused methods:
   - getRandomAvailableTCPPortRange(int)
   - getRandomAvailableTCPPorts(int,bool)
   - getRandomAvailableTCPPortRangeKeepers(int)
   - getRandomAvailableTCPPortRangeKeepers(int,bool)
   
   Removed two methods that used an ineffective, incorrect calculation to
   try to distribute ports evenly across VMs:
   
   - getRandomAvailablePortForDUnitSite()
   - getRandomAvailableTCPPortsForDUnitSite()
   
   These methods attempted to distribute ports by using the VM id as a
   modulus. The intention was something like this (assuming 5 VMs):
   
   VM 1 would get ports 20001, 20006, 20011, 20016, ...
   VM 2 would get ports 20002, 20007, 20012, 20017, ...
   VM 3 would get ports 20003, 20008, 20013, 20018, ...
   VM 4 would get ports 20004, 20009, 20014, 20019, ...
   VM 5 would get ports 2, 20005, 20010, 20015, ...
   
   But the actual calculation distributed ports like this:
   
   VM 1: 2, 20001, 20002, 20003, 20004, ...
   VM 2: 2, 20002, 20004, 20006, 20008, ...
   VM 3: 20001, 20004, 20007, 20010, 20013, ...
   VM 4: 2, 20004, 20008, 20012, 20016, ...
   VM 5: 2, 20005, 20010, 20015, 20020, ...
   
   ... with lots of potential port collisions from one VM to another.
   
   The few uses of these methods were replaced by calls to existing methods
   getRandomAvailableTCPPort() and getRandomAvailableTCPPorts(), which
   offer a more reliable method of distributing ports.
   
   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, check Concourse for build 
issues and
   submit an update to your PR as soon as possible. If you need help, please 
send an
   email to d...@geode.apache.org.
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] demery-pivotal closed pull request #5471: GEODE-8404: Simplify AvailablePortHelper

[GitBox]

demery-pivotal closed pull request #5471:
URL: https://github.com/apache/geode/pull/5471


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] agingade commented on a change in pull request #5492: GEODE-8475: Resolve a potential dead lock in ParallelGatewaySenderQueue

[GitBox]

agingade commented on a change in pull request #5492:
URL: https://github.com/apache/geode/pull/5492#discussion_r481287853



##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##
@@ -755,12 +755,16 @@ public boolean put(Object object) throws 
InterruptedException, CacheException {
 bucketFullPath, brq);
   }
   if (brq != null) {
+boolean intializingLocked = brq.lockWhenRegionIsInitializing();

Review comment:
   Can we add unit tests to make sure failed Initialization lock is held 
during put. 

##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##
@@ -755,12 +755,16 @@ public boolean put(Object object) throws 
InterruptedException, CacheException {
 bucketFullPath, brq);
   }
   if (brq != null) {
+boolean intializingLocked = brq.lockWhenRegionIsInitializing();
 brq.getInitializationLock().readLock().lock();
 try {
   putIntoBucketRegionQueue(brq, key, value);
   putDone = true;
 } finally {
   brq.getInitializationLock().readLock().unlock();
+  if (intializingLocked) {

Review comment:
   I assume we don't have to worry about the above unlock code throwing any 
exception...





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] bschuchardt closed pull request #5363: GEODE-8349: Reintroduce use of SSLSocket in cluster communications

[GitBox]

bschuchardt closed pull request #5363:
URL: https://github.com/apache/geode/pull/5363


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] bschuchardt merged pull request #5490: GEODE-8467: server fails to notify of a ForcedDisconnect and fails to tear down the cache

[GitBox]

bschuchardt merged pull request #5490:
URL: https://github.com/apache/geode/pull/5490


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] mkevo merged pull request #5476: GEODE-8455: Fix difference between create region with gw sender and a…

[GitBox]

mkevo merged pull request #5476:
URL: https://github.com/apache/geode/pull/5476


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] gesterzhou opened a new pull request #5492: GEODE-8475: Resolve a potential dead lock in ParallelGatewaySenderQueue

[GitBox]

gesterzhou opened a new pull request #5492:
URL: https://github.com/apache/geode/pull/5492


   Co-authored-by: Darrel Schneider 
   
   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, check Concourse for build 
issues and
   submit an update to your PR as soon as possible. If you need help, please 
send an
   email to d...@geode.apache.org.
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org