You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/12/04 15:49:41 UTC

[GitHub] [geode-native] jvarenina opened a new pull request #705: Feature/geode 8543

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


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #705:
URL: https://github.com/apache/geode-native/pull/705#discussion_r567989848



##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -37,6 +37,8 @@ namespace geode {
 namespace client {
 
 const char* TcrEndpoint::NC_Notification = "NC Notification";
+volatile bool TcrEndpoint::TEST_DISCONNECTIONS = false;

Review comment:
       If we are going to use a global variable, or even if this becomes and instance variable on `CacheImpl` we should use `std::atomic_flag` here.
   
   Good writeup on why here: https://www.drdobbs.com/parallel/volatile-vs-volatile/212701484

##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -1155,6 +1157,10 @@ void TcrEndpoint::setConnectionStatus(bool status) {
       m_isActiveEndpoint = false;
       LOGFINE("Disconnected from endpoint %s", m_name.c_str());
       triggerRedundancyThread();
+      // Test hook
+      if (TcrEndpoint::TEST_DISCONNECTIONS) {
+        TcrEndpoint::listOfDisconnectedServers.push_back(m_name.c_str());

Review comment:
       If you are adding `std::string` to this vector then you should not be converting `m_name` to a `C-string` first.
   ```c++
   TcrEndpoint::listOfDisconnectedServers.push_back(m_name);
   ```
   

##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -1155,6 +1157,10 @@ void TcrEndpoint::setConnectionStatus(bool status) {
       m_isActiveEndpoint = false;
       LOGFINE("Disconnected from endpoint %s", m_name.c_str());
       triggerRedundancyThread();
+      // Test hook
+      if (TcrEndpoint::TEST_DISCONNECTIONS) {
+        TcrEndpoint::listOfDisconnectedServers.push_back(m_name.c_str());

Review comment:
       I am strongly against adding any static methods and variables without a really strong argument for why they can't be member instance variables.
   
   I think it would make for a cleaner solution to and an API to the owner of `TcrEndpoint` collections, I think this might be `CacheImpl`, that allows for the registration of an event handler. Then on any of these `TcrEndpoint` status changes we would invoke the registered handlers. The test could then register handler it needs and asserts that it is invoked, without the need for global variables.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] codecov-io edited a comment on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-754919087


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=h1) Report
   > Merging [#705](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=desc) (12d7a95) into [develop](https://codecov.io/gh/apache/geode-native/commit/4dd2ceb9ef0dd081039bad76cdb78a3a6cc0a2fd?el=desc) (4dd2ceb) will **decrease** coverage by `0.02%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/705/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #705      +/-   ##
   ===========================================
   - Coverage    74.09%   74.06%   -0.03%     
   ===========================================
     Files          642      642              
     Lines        50812    50812              
   ===========================================
   - Hits         37647    37633      -14     
   - Misses       13165    13179      +14     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cppcache/src/ThinClientBaseDM.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRCYXNlRE0uY3Bw) | `68.33% <100.00%> (-0.56%)` | :arrow_down: |
   | [...test/testThinClientPoolExecuteHAFunctionPrSHOP.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFBvb2xFeGVjdXRlSEFGdW5jdGlvblByU0hPUC5jcHA=) | `91.20% <0.00%> (-3.71%)` | :arrow_down: |
   | [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `62.92% <0.00%> (-2.75%)` | :arrow_down: |
   | [cppcache/src/ExecutionImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0V4ZWN1dGlvbkltcGwuY3Bw) | `69.92% <0.00%> (-0.40%)` | :arrow_down: |
   | [cppcache/src/ThinClientRedundancyManager.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWR1bmRhbmN5TWFuYWdlci5jcHA=) | `75.74% <0.00%> (-0.16%)` | :arrow_down: |
   | [cppcache/src/ThinClientRegion.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWdpb24uY3Bw) | `56.28% <0.00%> (-0.06%)` | :arrow_down: |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `77.38% <0.00%> (+0.07%)` | :arrow_up: |
   | [cppcache/src/ThinClientLocatorHelper.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRMb2NhdG9ySGVscGVyLmNwcA==) | `93.57% <0.00%> (+0.71%)` | :arrow_up: |
   | [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `56.47% <0.00%> (+0.98%)` | :arrow_up: |
   | ... and [1 more](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=footer). Last update [4dd2ceb...12d7a95](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] echobravopapa commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
echobravopapa commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-809749297


   @jvarenina you might consider closing this PR until it becomes a priority to finish, thanks for keeping geode-native tidy


-- 
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:
users@infra.apache.org

[GitHub] [geode-native] jvarenina commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
jvarenina commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-768466580


   > @jvarenina Any update on this one? Still no new commits on the branch...
   
   Sorry, it seems that I neglect this commit. I will put focus on it tomorrow and update it. Thanks on the reminder!


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] codecov-io edited a comment on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-754919087


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=h1) Report
   > Merging [#705](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=desc) (12d7a95) into [develop](https://codecov.io/gh/apache/geode-native/commit/4dd2ceb9ef0dd081039bad76cdb78a3a6cc0a2fd?el=desc) (4dd2ceb) will **decrease** coverage by `0.03%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/705/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #705      +/-   ##
   ===========================================
   - Coverage    74.09%   74.05%   -0.04%     
   ===========================================
     Files          642      642              
     Lines        50812    50812              
   ===========================================
   - Hits         37647    37631      -16     
   - Misses       13165    13181      +16     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cppcache/src/ThinClientBaseDM.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRCYXNlRE0uY3Bw) | `68.33% <100.00%> (-0.56%)` | :arrow_down: |
   | [...test/testThinClientPoolExecuteHAFunctionPrSHOP.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFBvb2xFeGVjdXRlSEFGdW5jdGlvblByU0hPUC5jcHA=) | `91.20% <0.00%> (-3.71%)` | :arrow_down: |
   | [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `62.47% <0.00%> (-3.21%)` | :arrow_down: |
   | [cppcache/src/ExecutionImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0V4ZWN1dGlvbkltcGwuY3Bw) | `69.92% <0.00%> (-0.40%)` | :arrow_down: |
   | [cppcache/src/ThinClientRedundancyManager.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWR1bmRhbmN5TWFuYWdlci5jcHA=) | `75.74% <0.00%> (-0.16%)` | :arrow_down: |
   | [cppcache/src/ThinClientRegion.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWdpb24uY3Bw) | `56.28% <0.00%> (-0.06%)` | :arrow_down: |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `77.38% <0.00%> (+0.07%)` | :arrow_up: |
   | [cppcache/src/ThinClientLocatorHelper.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRMb2NhdG9ySGVscGVyLmNwcA==) | `93.57% <0.00%> (+0.71%)` | :arrow_up: |
   | [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `56.47% <0.00%> (+0.98%)` | :arrow_up: |
   | ... and [1 more](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=footer). Last update [4dd2ceb...12d7a95](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-770967469


   On the bright side, Windows build issues are fixed now.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] jvarenina commented on a change in pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #705:
URL: https://github.com/apache/geode-native/pull/705#discussion_r569326130



##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -1155,6 +1157,10 @@ void TcrEndpoint::setConnectionStatus(bool status) {
       m_isActiveEndpoint = false;
       LOGFINE("Disconnected from endpoint %s", m_name.c_str());
       triggerRedundancyThread();
+      // Test hook
+      if (TcrEndpoint::TEST_DISCONNECTIONS) {
+        TcrEndpoint::listOfDisconnectedServers.push_back(m_name.c_str());

Review comment:
       Thanks for comments!
   
   Would it be acceptable to use existing counter and method `numberOfTimesFailed()` from `TcrEndpoint` to check for disconnections?
   
   ```
   int32_t numberOfTimesFailed() { return m_numberOfTimesFailed; }
    ...
   void TcrEndpoint::setConnectionStatus(bool status) {
     std::lock_guard<decltype(m_connectionLock)> guard(m_connectionLock);
       if (m_connected != status) {
         bool connected = m_connected;
         m_connected = status;
         if (connected) {
           m_numberOfTimesFailed += 1;
           m_isAuthenticated = false;
           // disconnected
           LOGFINE("Disconnecting from endpoint %s", m_name.c_str());
           closeConnections();
           ...
   ```
   I think it would be possible to use it, because `TcrEndpoint` object is not destroyed when endpoint disconnects from client.
   
   It would be used in test case something like this:
   ```
   auto cacheImpl = CacheRegionHelper::getCacheImpl(cache.get());
   
   // shut down one endpoint (server)
   
   ASSERT_EQ(cacheImpl->getNumberOfTimeEndpointDisconnected(poolName, epRunningHostname), 0);
   ASSERT_EQ(cacheImpl->getNumberOfTimeEndpointDisconnected(poolName, epDisconnectedHostname), 1);
   
   ```
   `ThinClientPoolDM` is actually owner of `TcrEndpoint` collection, so `getNumberOfTimeEndpointDisconnected(pool, endpoint)` would basically find given endpoint and return it's `numberOfTimesFailed()`.
   
   
   




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #705:
URL: https://github.com/apache/geode-native/pull/705#discussion_r544472676



##########
File path: cppcache/integration/test/DisconnectEndPointAtException.cpp
##########
@@ -0,0 +1,195 @@
+/*
+ * 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.
+ */
+#include <framework/Cluster.h>
+#include <framework/Gfsh.h>
+#include <framework/TestConfig.h>
+
+#include <thread>
+
+#include <geode/Cache.hpp>
+#include <geode/CacheFactory.hpp>
+#include <geode/CacheTransactionManager.hpp>
+#include <geode/Region.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+namespace {
+using apache::geode::client::Cache;
+using apache::geode::client::CacheableString;
+using apache::geode::client::CacheFactory;
+using apache::geode::client::CacheTransactionManager;
+using apache::geode::client::Pool;
+using apache::geode::client::Region;
+using apache::geode::client::RegionShortcut;
+using std::cout;
+
+std::string disconnectingStr("Disconnecting from endpoint");
+
+std::string getClientLogName() {

Review comment:
       Using log output is not a safe way to write a test. Is there something else we can hook to test the disconnect scenario? 
   
   Could this code change not be refactored for a unit test?
   Could we add hooks on some internal interface for getting events server disconnects?




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-766921573


   @jvarenina Any update on this one?  Still no new commits on the branch...
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] jvarenina commented on a change in pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #705:
URL: https://github.com/apache/geode-native/pull/705#discussion_r569326130



##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -1155,6 +1157,10 @@ void TcrEndpoint::setConnectionStatus(bool status) {
       m_isActiveEndpoint = false;
       LOGFINE("Disconnected from endpoint %s", m_name.c_str());
       triggerRedundancyThread();
+      // Test hook
+      if (TcrEndpoint::TEST_DISCONNECTIONS) {
+        TcrEndpoint::listOfDisconnectedServers.push_back(m_name.c_str());

Review comment:
       Thanks for comments!
   
   Would it be acceptable to use existing counter and method `numberOfTimesFailed()` from `TcrEndpoint` to check for disconnections?
   
   ```
   int32_t numberOfTimesFailed() { return m_numberOfTimesFailed; }
    ...
   void TcrEndpoint::setConnectionStatus(bool status) {
     std::lock_guard<decltype(m_connectionLock)> guard(m_connectionLock);
       if (m_connected != status) {
         bool connected = m_connected;
         m_connected = status;
         if (connected) {
           m_numberOfTimesFailed += 1;
           m_isAuthenticated = false;
           // disconnected
           LOGFINE("Disconnecting from endpoint %s", m_name.c_str());
           closeConnections();
           ...
   ```
   I think it would be possible to use it, because `TcrEndpoint` object is not destroyed when endpoint disconnects from client.
   
   It would be used in test case something like this:
   ```
   auto cacheImpl = CacheRegionHelper::getCacheImpl(cache.get());
   
   // shut down one endpoint (server)
   
   ASSERT_EQ(cacheImpl->getNumberOfTimeEndpointDisconnected(poolName, epRunningHostname), 0);
   ASSERT_EQ(cacheImpl->getNumberOfTimeEndpointDisconnected(poolName, epDisconnectedHostname), 1);
   
   ```
   `ThinClientPoolDM` is actually owner of `TcrEndpoint` collection, so `getNumberOfTimeEndpointDisconnected(pool, endpoint)` would basically find given endpoint and return it's `numberOfTimesFailed()`. If this is not accepatble, I'll then try to implement event handler.
   
   
   




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] jvarenina commented on a change in pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #705:
URL: https://github.com/apache/geode-native/pull/705#discussion_r569346657



##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -37,6 +37,8 @@ namespace geode {
 namespace client {
 
 const char* TcrEndpoint::NC_Notification = "NC Notification";
+volatile bool TcrEndpoint::TEST_DISCONNECTIONS = false;

Review comment:
       Thanks for the article!




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] codecov-io edited a comment on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-754919087


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=h1) Report
   > Merging [#705](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=desc) (12d7a95) into [develop](https://codecov.io/gh/apache/geode-native/commit/4dd2ceb9ef0dd081039bad76cdb78a3a6cc0a2fd?el=desc) (4dd2ceb) will **decrease** coverage by `0.03%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/705/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #705      +/-   ##
   ===========================================
   - Coverage    74.09%   74.05%   -0.04%     
   ===========================================
     Files          642      642              
     Lines        50812    50812              
   ===========================================
   - Hits         37647    37631      -16     
   - Misses       13165    13181      +16     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cppcache/src/ThinClientBaseDM.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRCYXNlRE0uY3Bw) | `68.33% <100.00%> (-0.56%)` | :arrow_down: |
   | [...test/testThinClientPoolExecuteHAFunctionPrSHOP.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFBvb2xFeGVjdXRlSEFGdW5jdGlvblByU0hPUC5jcHA=) | `91.20% <0.00%> (-3.71%)` | :arrow_down: |
   | [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `62.47% <0.00%> (-3.21%)` | :arrow_down: |
   | [cppcache/src/ExecutionImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0V4ZWN1dGlvbkltcGwuY3Bw) | `69.92% <0.00%> (-0.40%)` | :arrow_down: |
   | [cppcache/src/ThinClientRedundancyManager.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWR1bmRhbmN5TWFuYWdlci5jcHA=) | `75.74% <0.00%> (-0.16%)` | :arrow_down: |
   | [cppcache/src/ThinClientRegion.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWdpb24uY3Bw) | `56.28% <0.00%> (-0.06%)` | :arrow_down: |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `77.38% <0.00%> (+0.07%)` | :arrow_up: |
   | [cppcache/src/ThinClientLocatorHelper.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRMb2NhdG9ySGVscGVyLmNwcA==) | `93.57% <0.00%> (+0.71%)` | :arrow_up: |
   | [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `56.47% <0.00%> (+0.98%)` | :arrow_up: |
   | ... and [1 more](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=footer). Last update [4dd2ceb...12d7a95](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-810381412


   @jvarenina Looks like `clang-format` is still an issue with the one file.  FYI, the CI is using `clang-format` v11 now, so perhaps your local version is an issue.
   
   Windows 2019 build has a stuck integration test (see below).  This isn't a test known for hanging/crashing, so I recommend pushing the formatting fix and waiting for the next build before investigating.
   ----------------------------
   
   Passed:
       testCacheless (#1)
       testCreateAndDestroyPool (#2)
       testDataOutput (#3)
       testDunit (#4)
       testExpiration (#5)
       testFWHelper (#6)
       testLinkage (#8)
       testPdxMetadataCheckTest (#10)
       testPoolFactory (#11)
       testRegionAccessThreadSafe (#12)
       testRegionAttributesFactory (#13)
       testRegionMap (#14)
       testRegionTemplateArgs (#15)
       testSerialization (#16)
       testSpinLock (#17)
       testSubRegions (#18)
       testSystemProperties (#19)
       testThinClientBigValue (#21)
       testThinClientCacheableStringArray (#22)
       testThinClientCacheablesLimits (#24)
       testThinClientCallbackArg (#25)
       testThinClientClearRegion (#26)
       testThinClientConflation (#27)
       testThinClientContainsKeyOnServer (#28)
       testThinClientCqDelta (#30)
       testThinClientCqIR (#34)
       testThinClientDeltaWithNotification (#35)
       testThinClientDisconnectionListioner (#36)
       testThinClientDistOps2 (#37)
       testThinClientDistOpsDontUpdateLocatorList (#38)
       testThinClientDistOpsNotSticky (#39)
       testThinClientDistOpsSticky (#40)
       testThinClientDistOpsUpdateLocatorList (#41)
       testThinClientDurableCrashNormal (#43)
       testThinClientDurableCrashTimeout (#44)
       testThinClientDurableFailoverClientClosedRedundancy (#48)
       testThinClientDurableInterest (#50)
       testThinClientDurableKeepAliveFalseNormal (#51)
       testThinClientDurableKeepAliveTrueTimeout (#54)
       testThinClientFailover3 (#58)
       testThinClientFailoverInterestAllWithCache (#61)
       testThinClientGetInterests (#65)
       testThinClientHAPeriodicAck (#71)
       testThinClientHeapLRU (#73)
       testThinClientIntResPolKeysInv (#74)
       testThinClientInterest1 (#75)
       testThinClientInterest1Cacheless (#76)
       testThinClientInterest1_Bug1001 (#77)
       testThinClientInterest2Pooled (#78)
       testThinClientInterest3 (#79)
       testThinClientInterest3Cacheless (#80)
       testThinClientInterestList (#81)
       testThinClientInterestList2 (#82)
       testThinClientInterestNotify (#83)
       testThinClientListenerCallbackArgTest (#86)
       testThinClientListenerEvents (#87)
       testThinClientListenerInit (#88)
       testThinClientListenerWriterWithSubscription (#89)
       testThinClientListenerWriterWithoutSubscription (#90)
       testThinClientLocalCacheLoader (#91)
       testThinClientLocator (#92)
       testThinClientMultipleCaches (#95)
       testThinClientNotification (#96)
       testThinClientNotificationWithDeltaWithoutcache (#97)
       testThinClientPRSingleHopServerGroup (#99)
       testThinClientPdxEnum (#102)
       testThinClientPdxInstance (#103)
       testThinClientPdxSerializer (#104)
       testThinClientPdxSerializerForJava (#105)
       testThinClientPoolExecuteFunctionThrowsException (#108)
       testThinClientPoolExecuteHAFunction (#109)
       testThinClientPoolExecuteHAFunctionPrSHOP (#110)
       testThinClientPoolRegInterest (#113)
       testThinClientPutAllTimeout (#116)
       testThinClientPutAllWithCallBackArgWithConcurrency (#117)
       testThinClientPutGetAll (#119)
       testThinClientRIwithlocalRegionDestroy (#121)
       testThinClientRegex (#122)
       testThinClientRegex2 (#123)
       testThinClientRegex3 (#124)
       testThinClientRegionQueryDifferentServerConfigs (#125)
       testThinClientRegionQueryExclusiveness (#126)
       testThinClientRemoteQueryFailover (#127)
       testThinClientRemoteQueryFailoverPdx (#128)
       testThinClientRemoteQueryRS (#129)
       testThinClientRemoteQuerySS (#130)
       testThinClientRemoteRegionQuery (#132)
       testThinClientRemoveAll (#133)
       testThinClientRemoveAllLocal (#134)
       testThinClientRemoveAllSequence (#135)
       testThinClientSecurityAuthenticationSetAuthInitialize (#139)
       testThinClientSecurityMultiUserTest (#144)
       testThinClientTicket317 (#149)
       testThinClientTransactionsWithSticky (#151)
       testThinClientTransactionsWithoutSticky (#152)
       testThinClientVersionedOps (#154)
       testThinClientVersionedOpsPartitionPersistent (#155)
       testThinClientVersionedOpsReplicate (#156)
       testThinClientVersionedOpsReplicatePersistent (#157)
       testXmlCacheCreationWithOverFlow (#160)
       testXmlCacheCreationWithPools (#161)
       testXmlCacheCreationWithRefid (#162)
       testXmlCacheInitialization (#163)
   Failed:
       testThinClientSecurityMultiUserTest (#144)
   Timed Out:
       None
   Not Run:
       testFwPerf (#7)
       testOverflowPutGetSqLite (#9)
       testThinClientAfterRegionLive (#20)
       testThinClientCacheables (#23)
       testThinClientCq (#29)
       testThinClientCqDurable (#31)
       testThinClientCqFailover (#32)
       testThinClientCqHAFailover (#33)
       testThinClientDurableConnect (#42)
       testThinClientDurableDisconnectNormal (#45)
       testThinClientDurableDisconnectTimeout (#46)
       testThinClientDurableFailoverClientClosedNoRedundancy (#47)
       testThinClientDurableFailoverClientNotClosedRedundancy (#49)
       testThinClientDurableKeepAliveFalseTimeout (#52)
       testThinClientDurableKeepAliveTrueNormal (#53)
       testThinClientDurableReconnect (#55)
       testThinClientFailover (#56)
       testThinClientFailover2 (#57)
       testThinClientFailoverInterest (#59)
       testThinClientFailoverInterest2 (#60)
       testThinClientFailoverRegex (#62)
       testThinClientFixedPartitionResolver (#63)
       testThinClientGatewayTest (#64)
       testThinClientHADistOps (#66)
       testThinClientHAEventIDMap (#67)
       testThinClientHAFailover (#68)
       testThinClientHAFailoverRegex (#69)
       testThinClientHAMixedRedundancy (#70)
       testThinClientHAQueryFailover (#72)
       testThinClientLRUExpiration (#84)
       testThinClientLargePutAllWithCallBackArg (#85)
       testThinClientLocatorFailover (#93)
       testThinClientMultiDS (#94)
       testThinClientPRPutAllFailover (#98)
       testThinClientPartitionResolver (#100)
       testThinClientPdxDeltaWithNotification (#101)
       testThinClientPdxTests (#106)
       testThinClientPoolAttrTest (#107)
       testThinClientPoolLocator (#111)
       testThinClientPoolRedundancy (#112)
       testThinClientPoolServer (#114)
       testThinClientPutAll (#115)
       testThinClientPutAllWithCallBackArgWithoutConcurrency (#118)
       testThinClientRemoteQueryTimeout (#131)
       testThinClientRemoveOps (#136)
       testThinClientSecurityAuthentication (#137)
       testThinClientSecurityAuthenticationMU (#138)
       testThinClientSecurityAuthorization (#140)
       testThinClientSecurityAuthorizationMU (#141)
       testThinClientSecurityCQAuthorizationMU (#142)
       testThinClientSecurityDurableCQAuthorizationMU (#143)
       testThinClientSecurityPostAuthorization (#145)
       testThinClientTXFailover (#146)
       testThinClientTicket303 (#147)
       testThinClientTicket304 (#148)
       testThinClientTracking (#150)
       testThinClientTransactionsXA (#153)
       testThinClientWriterException (#158)
       testTimedSemaphore (#159)
   Stuck:
       testThinClientPutWithDelta (#120)
   Tests Passed:
       103/105 (98%)
   Tests Failed:
       1/105 (1%)
   Tests Timed Out:
       0/105 (0%)
   Tests Stuck:
       1/105 (1%)
   Tests Not Run:
       59/164 (36%)
   


-- 
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:
users@infra.apache.org

[GitHub] [geode-native] jvarenina commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
jvarenina commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-810896161


   Hi @pdxcodemonkey , thank you for the review and the helpful hint (I had clang-format v6). I have formatted and pushed the code. Lets hope that test on Windows will pass this time :)


-- 
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:
users@infra.apache.org

[GitHub] [geode-native] jvarenina commented on a change in pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #705:
URL: https://github.com/apache/geode-native/pull/705#discussion_r569326130



##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -1155,6 +1157,10 @@ void TcrEndpoint::setConnectionStatus(bool status) {
       m_isActiveEndpoint = false;
       LOGFINE("Disconnected from endpoint %s", m_name.c_str());
       triggerRedundancyThread();
+      // Test hook
+      if (TcrEndpoint::TEST_DISCONNECTIONS) {
+        TcrEndpoint::listOfDisconnectedServers.push_back(m_name.c_str());

Review comment:
       Thanks for comments!
   
   Would it be acceptable to use existing counter and method `numberOfTimesFailed()` from `TcrEndpoint` to check for disconnections?
   
   ```
   int32_t numberOfTimesFailed() { return m_numberOfTimesFailed; }
    ...
   void TcrEndpoint::setConnectionStatus(bool status) {
     std::lock_guard<decltype(m_connectionLock)> guard(m_connectionLock);
       if (m_connected != status) {
         bool connected = m_connected;
         m_connected = status;
         if (connected) {
           m_numberOfTimesFailed += 1;
           m_isAuthenticated = false;
           // disconnected
           LOGFINE("Disconnecting from endpoint %s", m_name.c_str());
           closeConnections();
           ...
   ```
   I think it would be possible to use it, because `TcrEndpoint` object is not destroyed when endpoint disconnects from client.
   
   It would be used in test case something like this:
   ```
   auto cacheImpl = CacheRegionHelper::getCacheImpl(cache.get());
   
   // shut down one endpoint (server)
   
   ASSERT_EQ(cacheImpl->getNumberOfTimeEndpointDisconnected(poolName, epRunningHostname), 0);
   ASSERT_EQ(cacheImpl->getNumberOfTimeEndpointDisconnected(poolName, epDisconnectedHostname), 1);
   
   ```
   `ThinClientPoolDM` is actually owner of `TcrEndpoint` collection, so `getNumberOfTimeEndpointDisconnected(endpoint)` would basically find given endpoint and return it's `numberOfTimesFailed()`.
   
   
   




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey merged pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #705:
URL: https://github.com/apache/geode-native/pull/705


   


-- 
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:
users@infra.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #705:
URL: https://github.com/apache/geode-native/pull/705#discussion_r567922837



##########
File path: cppcache/integration/framework/Cluster.cpp
##########
@@ -161,7 +163,7 @@ Server::Server(Cluster &cluster, std::vector<Locator> &locators,
     hostname = "ip6-localhost";
   }
 
-  auto port = static_cast<uint16_t>(0);
+  auto port = Framework::getAvailablePort();

Review comment:
       Geode server will select its own ephemeral port if we send 0. We should preserve this default behavior in the tests. If you test needs to know the specific port of the server then that test should use this method to get a free port and assign it to the server in the cluster config. 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] jvarenina commented on a change in pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #705:
URL: https://github.com/apache/geode-native/pull/705#discussion_r545159283



##########
File path: cppcache/integration/test/DisconnectEndPointAtException.cpp
##########
@@ -0,0 +1,195 @@
+/*
+ * 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.
+ */
+#include <framework/Cluster.h>
+#include <framework/Gfsh.h>
+#include <framework/TestConfig.h>
+
+#include <thread>
+
+#include <geode/Cache.hpp>
+#include <geode/CacheFactory.hpp>
+#include <geode/CacheTransactionManager.hpp>
+#include <geode/Region.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+namespace {
+using apache::geode::client::Cache;
+using apache::geode::client::CacheableString;
+using apache::geode::client::CacheFactory;
+using apache::geode::client::CacheTransactionManager;
+using apache::geode::client::Pool;
+using apache::geode::client::Region;
+using apache::geode::client::RegionShortcut;
+using std::cout;
+
+std::string disconnectingStr("Disconnecting from endpoint");
+
+std::string getClientLogName() {

Review comment:
       Thank you for your suggestions. We will investigate, if it is possible to implement some of them.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-810502347


   > Looks good. Appreciate the work getting the testing hooks in so we don't have to parse logs.
   
   This is ready to merge as soon as formatting is fixed.  Unless the Windows test fails again, but I think that's unlikely.


-- 
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:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-766921573


   @jvarenina Any update on this one?  Still no new commits on the branch...
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] jvarenina commented on a change in pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #705:
URL: https://github.com/apache/geode-native/pull/705#discussion_r569326130



##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -1155,6 +1157,10 @@ void TcrEndpoint::setConnectionStatus(bool status) {
       m_isActiveEndpoint = false;
       LOGFINE("Disconnected from endpoint %s", m_name.c_str());
       triggerRedundancyThread();
+      // Test hook
+      if (TcrEndpoint::TEST_DISCONNECTIONS) {
+        TcrEndpoint::listOfDisconnectedServers.push_back(m_name.c_str());

Review comment:
       Thanks for comments!
   
   Would it be acceptable to use existing counter and method `numberOfTimesFailed()` from `TcrEndpoint` to check for disconnections?
   
   ```
   int32_t numberOfTimesFailed() { return m_numberOfTimesFailed; }
    ...
   void TcrEndpoint::setConnectionStatus(bool status) {
     std::lock_guard<decltype(m_connectionLock)> guard(m_connectionLock);
       if (m_connected != status) {
         bool connected = m_connected;
         m_connected = status;
         if (connected) {
           m_numberOfTimesFailed += 1;
           m_isAuthenticated = false;
           // disconnected
           LOGFINE("Disconnecting from endpoint %s", m_name.c_str());
           closeConnections();
           ...
   ```
   I think it would be possible to use it, because `TcrEndpoint` object is not destroyed when endpoint disconnects from client.
   
   It would be used in test case something like this:
   ```
   auto cacheImpl = CacheRegionHelper::getCacheImpl(cache.get());
   
   // shut down one endpoint (server)
   
   ASSERT_EQ(cacheImpl->getNumberOfTimeEndpointDisconnected(poolName, epRunningHostname), 0);
   ASSERT_GT(cacheImpl->getNumberOfTimeEndpointDisconnected(poolName, epDisconnectedHostname), 0);
   
   ```
   `ThinClientPoolDM` is actually owner of `TcrEndpoint` collection, so `getNumberOfTimeEndpointDisconnected(endpoint)` would basically find given endpoint and return it's `numberOfTimesFailed()`.
   
   
   




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-755445765


   @jvarenina  I'm getting build errors on Windows:
   ```
   C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(54): error C2079: 'previousTestLog' uses undefined class 'std::basic_ifstream<char,std::char_traits<char>>' [C:\build\cppcache\integration\test\cpp-integration-test.vcxproj]
     Building Custom Rule C:/nativeclient/cppcache/integration-test/CMakeLists.txt
   C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(54): error C2440: 'initializing': cannot convert from 'std::string' to 'int' [C:\build\cppcache\integration\test\cpp-integration-test.vcxproj]
     C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(54): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
   C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(168): error C2079: 'testLog' uses undefined class 'std::basic_ifstream<char,std::char_traits<char>>' [C:\build\cppcache\integration\test\cpp-integration-test.vcxproj]
   C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(168): error C2440: 'initializing': cannot convert from 'std::string' to 'int' [C:\build\cppcache\integration\test\cpp-integration-test.vcxproj]
     C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(168): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
   C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(174): error C2672: 'getline': no matching overloaded function found [C:\build\cppcache\integration\test\cpp-integration-test.vcxproj]
     testOverflowPutGetSqLite.cpp
   C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(174): error C2784: 'std::basic_istream<_Elem,_Traits> &std::getline(std::basic_istream<_Elem,_Traits> &,std::basic_string<_Elem,_Traits,_Alloc> &)': could not deduce template argument for 'std::basic_istream<_Elem,_Traits> &' from 'int' [C:\build\cppcache\integration\test\cpp-integration-test.vcxproj]
     Building Custom Rule C:/nativeclient/cppcache/integration-test/CMakeLists.txt
     testThinClientInterest1Cacheless.cpp
     C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.15.26726\include\string(160): note: see declaration of 'std::getline'
   C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(174): error C2780: 'std::basic_istream<_Elem,_Traits> &std::getline(std::basic_istream<_Elem,_Traits> &,std::basic_string<_Elem,_Traits,_Alloc> &,const _Elem)': expects 3 arguments - 2 provided [C:\build\cppcache\integration\test\cpp-integration-test.vcxproj]
     C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.15.26726\include\string(149): note: see declaration of 'std::getline'
     testThinClientRegex3.cpp
   C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(174): error C2784: 'std::basic_istream<_Elem,_Traits> &std::getline(std::basic_istream<_Elem,_Traits> &&,std::basic_string<_Elem,_Traits,_Alloc> &)': could not deduce template argument for 'std::basic_istream<_Elem,_Traits> &&' from 'int' [C:\build\cppcache\integration\test\cpp-integration-test.vcxproj]
     C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.15.26726\include\string(129): note: see declaration of 'std::getline'
   C:\nativeclient\cppcache\integration\test\DisconnectEndPointAtException.cpp(174): error C2780: 'std::basic_istream<_Elem,_Traits> &std::getline(std::basic_istream<_Elem,_Traits> &&,std::basic_string<_Elem,_Traits,_Alloc> &,const _Elem)': expects 3 arguments - 2 provided [C:\build\cppcache\integration\test\cpp-integration-test.vcxproj]
     C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.15.26726\include\string(76): note: see declaration of 'std::getline'
     testThinClientLargePutAllWithCallBackArg.cpp
   ```


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] codecov-io commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-754919087


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=h1) Report
   > Merging [#705](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=desc) (12d7a95) into [develop](https://codecov.io/gh/apache/geode-native/commit/4dd2ceb9ef0dd081039bad76cdb78a3a6cc0a2fd?el=desc) (4dd2ceb) will **decrease** coverage by `0.08%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/705/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #705      +/-   ##
   ===========================================
   - Coverage    74.09%   74.00%   -0.09%     
   ===========================================
     Files          642      642              
     Lines        50812    50812              
   ===========================================
   - Hits         37647    37604      -43     
   - Misses       13165    13208      +43     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cppcache/src/ThinClientBaseDM.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRCYXNlRE0uY3Bw) | `68.33% <100.00%> (-0.56%)` | :arrow_down: |
   | [cppcache/src/ReadWriteLock.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1JlYWRXcml0ZUxvY2suY3Bw) | `81.25% <0.00%> (-18.75%)` | :arrow_down: |
   | [...test/testThinClientPoolExecuteHAFunctionPrSHOP.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFBvb2xFeGVjdXRlSEFGdW5jdGlvblByU0hPUC5jcHA=) | `91.20% <0.00%> (-3.71%)` | :arrow_down: |
   | [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `62.47% <0.00%> (-3.21%)` | :arrow_down: |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `76.46% <0.00%> (-0.86%)` | :arrow_down: |
   | [cppcache/src/ThinClientRedundancyManager.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWR1bmRhbmN5TWFuYWdlci5jcHA=) | `75.27% <0.00%> (-0.63%)` | :arrow_down: |
   | [cppcache/src/ExecutionImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0V4ZWN1dGlvbkltcGwuY3Bw) | `69.92% <0.00%> (-0.40%)` | :arrow_down: |
   | [cppcache/src/ThinClientRegion.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWdpb24uY3Bw) | `56.28% <0.00%> (-0.06%)` | :arrow_down: |
   | [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `55.49% <0.00%> (ø)` | |
   | ... and [2 more](https://codecov.io/gh/apache/geode-native/pull/705/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=footer). Last update [4dd2ceb...12d7a95](https://codecov.io/gh/apache/geode-native/pull/705?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [geode-native] jvarenina commented on pull request #705: GEODE-8543: Fix for uneccessary connections closure

Posted by GitBox <gi...@apache.org>.
jvarenina commented on pull request #705:
URL: https://github.com/apache/geode-native/pull/705#issuecomment-756187587


   @pdxcodemonkey thanks for the info and test. It seems all errors are related to the part of code that is scraping logs. We will remove this code as proposed by Jacob Barret. We will soon push new code that should fix windows errors.


----------------------------------------------------------------
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:
users@infra.apache.org