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/11/11 08:01:00 UTC

[GitHub] [geode-native] albertogpz opened a new pull request #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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


   …cute with onServers
   
   When a function is executed by using onServers, if one of the servers goes down while the function is executed, no exception is thrown (despite what the Native C++ API documentation says).
   The reason was that when an IO error was detected when calling a function on a server, the FunctionService::execute() function instead of returning the IO error returned no error.
   With this change this function now returns the IO error which translates in a NotConnectedException thrown.


----------------------------------------------------------------
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 #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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


   


----------------------------------------------------------------
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 a change in pull request #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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



##########
File path: cppcache/src/ThinClientPoolDM.cpp
##########
@@ -2414,7 +2415,7 @@ GfErrType FunctionExecution::execute() {
   m_error = m_poolDM->sendRequestToEP(request, reply, m_ep);
   m_error = m_poolDM->handleEPError(m_ep, reply, m_error);
   if (m_error != GF_NOERR) {
-    if (m_error == GF_NOTCON || m_error == GF_IOERR) {
+    if (m_error == GF_NOTCON) {

Review comment:
       how did you determine we no longer need to check for`GF_IOERR`?

##########
File path: cppcache/src/ThinClientPoolDM.cpp
##########
@@ -670,11 +670,12 @@ GfErrType ThinClientPoolDM::sendRequestToAllServers(
         err == GF_NOT_AUTHORIZED_EXCEPTION ||
         err == GF_AUTHENTICATION_REQUIRED_EXCEPTION) {
       finalErrorReturn = err;
-    } else if (!(finalErrorReturn == GF_AUTHENTICATION_FAILED_EXCEPTION ||
-                 finalErrorReturn == GF_NOT_AUTHORIZED_EXCEPTION ||
-                 finalErrorReturn ==
-                     GF_AUTHENTICATION_REQUIRED_EXCEPTION))  // returning auth
-                                                             // errors
+    } else if ((err != GF_NOERR) &&

Review comment:
       the check for `!GF_NOERR` looks like a good addition




----------------------------------------------------------------
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] albertogpz commented on a change in pull request #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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



##########
File path: cppcache/src/ExecutionImpl.cpp
##########
@@ -429,33 +429,20 @@ void ExecutionImpl::executeOnAllServers(const std::string& func,
   std::shared_ptr<CacheableString> exceptionPtr = nullptr;
   GfErrType err = tcrdm->sendRequestToAllServers(
       func.c_str(), getResult, timeout, m_args, m_rc, exceptionPtr);
-  if (exceptionPtr != nullptr && err != GF_NOERR) {
-    LOGDEBUG("Execute errorred: %d", err);
-    // throw FunctionExecutionException( "Execute: failed to execute function
-    // with server." );
-    if (err == GF_CACHESERVER_EXCEPTION) {
-      throw FunctionExecutionException(
-          "Execute: failed to execute function with server.");
-    } else {
-      throwExceptionIfError("Execute", err);
-    }
-  }
-
-  if (err == GF_AUTHENTICATION_FAILED_EXCEPTION ||
-      err == GF_NOT_AUTHORIZED_EXCEPTION ||
-      err == GF_AUTHENTICATION_REQUIRED_EXCEPTION) {
-    throwExceptionIfError("Execute", err);
-  }
 
   if (err != GF_NOERR) {
+    LOGDEBUG("Execute errorred: %d", err);

Review comment:
       No problem. It looked weird to me too. I even thought the author might want to say something like "error red" :-).




----------------------------------------------------------------
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 #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=h1) Report
   > Merging [#690](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=desc) (a24d8c3) into [develop](https://codecov.io/gh/apache/geode-native/commit/5c7d47d34c2b8a53874ec6f53e66c2290fd0427c?el=desc) (5c7d47d) will **decrease** coverage by `0.04%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/690/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #690      +/-   ##
   ===========================================
   - Coverage    74.12%   74.07%   -0.05%     
   ===========================================
     Files          644      644              
     Lines        51187    51184       -3     
   ===========================================
   - Hits         37942    37916      -26     
   - Misses       13245    13268      +23     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cppcache/src/CqQueryImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NxUXVlcnlJbXBsLmNwcA==) | `56.37% <0.00%> (ø)` | |
   | [cppcache/src/ExecutionImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0V4ZWN1dGlvbkltcGwuY3Bw) | `69.92% <28.57%> (+1.84%)` | :arrow_up: |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `77.72% <100.00%> (+1.23%)` | :arrow_up: |
   | [cppcache/src/ThinClientStickyManager.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRTdGlja3lNYW5hZ2VyLmNwcA==) | `77.88% <0.00%> (-10.58%)` | :arrow_down: |
   | [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `54.27% <0.00%> (-2.39%)` | :arrow_down: |
   | [cppcache/src/TXCommitMessage.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RYQ29tbWl0TWVzc2FnZS5jcHA=) | `74.50% <0.00%> (-1.97%)` | :arrow_down: |
   | [cppcache/src/TcrConnection.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckNvbm5lY3Rpb24uY3Bw) | `72.48% <0.00%> (-1.26%)` | :arrow_down: |
   | [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `61.78% <0.00%> (-1.15%)` | :arrow_down: |
   | [...tegration-test/testThinClientRemoteRegionQuery.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFJlbW90ZVJlZ2lvblF1ZXJ5LmNwcA==) | `82.33% <0.00%> (-1.13%)` | :arrow_down: |
   | [cppcache/src/Log.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0xvZy5jcHA=) | `56.50% <0.00%> (-0.98%)` | :arrow_down: |
   | ... and [5 more](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/690?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/690?src=pr&el=footer). Last update [5c7d47d...a24d8c3](https://codecov.io/gh/apache/geode-native/pull/690?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] codecov-io commented on pull request #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=h1) Report
   > Merging [#690](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=desc) (bf89d76) into [develop](https://codecov.io/gh/apache/geode-native/commit/5c7d47d34c2b8a53874ec6f53e66c2290fd0427c?el=desc) (5c7d47d) will **decrease** coverage by `0.07%`.
   > The diff coverage is `54.54%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/690/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #690      +/-   ##
   ===========================================
   - Coverage    74.12%   74.04%   -0.08%     
   ===========================================
     Files          644      644              
     Lines        51187    51184       -3     
   ===========================================
   - Hits         37942    37899      -43     
   - Misses       13245    13285      +40     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cppcache/src/ExecutionImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0V4ZWN1dGlvbkltcGwuY3Bw) | `69.92% <28.57%> (+1.84%)` | :arrow_up: |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `76.71% <100.00%> (+0.23%)` | :arrow_up: |
   | [cppcache/src/ReadWriteLock.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1JlYWRXcml0ZUxvY2suY3Bw) | `81.25% <0.00%> (-18.75%)` | :arrow_down: |
   | [...tion-test/testThinClientRemoteQueryFailoverPdx.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFJlbW90ZVF1ZXJ5RmFpbG92ZXJQZHguY3Bw) | `85.48% <0.00%> (-4.04%)` | :arrow_down: |
   | [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `54.27% <0.00%> (-2.39%)` | :arrow_down: |
   | [cppcache/src/PdxTypeRegistry.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1BkeFR5cGVSZWdpc3RyeS5jcHA=) | `77.94% <0.00%> (-2.21%)` | :arrow_down: |
   | [cppcache/src/TXCommitMessage.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RYQ29tbWl0TWVzc2FnZS5jcHA=) | `74.50% <0.00%> (-1.97%)` | :arrow_down: |
   | [cppcache/src/TcrConnection.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckNvbm5lY3Rpb24uY3Bw) | `72.48% <0.00%> (-1.26%)` | :arrow_down: |
   | [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `61.78% <0.00%> (-1.15%)` | :arrow_down: |
   | [...tegration-test/testThinClientRemoteRegionQuery.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFJlbW90ZVJlZ2lvblF1ZXJ5LmNwcA==) | `82.33% <0.00%> (-1.13%)` | :arrow_down: |
   | ... and [8 more](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/690?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/690?src=pr&el=footer). Last update [5c7d47d...bf89d76](https://codecov.io/gh/apache/geode-native/pull/690?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 a change in pull request #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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



##########
File path: cppcache/src/ExecutionImpl.cpp
##########
@@ -429,33 +429,20 @@ void ExecutionImpl::executeOnAllServers(const std::string& func,
   std::shared_ptr<CacheableString> exceptionPtr = nullptr;
   GfErrType err = tcrdm->sendRequestToAllServers(
       func.c_str(), getResult, timeout, m_args, m_rc, exceptionPtr);
-  if (exceptionPtr != nullptr && err != GF_NOERR) {
-    LOGDEBUG("Execute errorred: %d", err);
-    // throw FunctionExecutionException( "Execute: failed to execute function
-    // with server." );
-    if (err == GF_CACHESERVER_EXCEPTION) {
-      throw FunctionExecutionException(
-          "Execute: failed to execute function with server.");
-    } else {
-      throwExceptionIfError("Execute", err);
-    }
-  }
-
-  if (err == GF_AUTHENTICATION_FAILED_EXCEPTION ||
-      err == GF_NOT_AUTHORIZED_EXCEPTION ||
-      err == GF_AUTHENTICATION_REQUIRED_EXCEPTION) {
-    throwExceptionIfError("Execute", err);
-  }
 
   if (err != GF_NOERR) {
+    LOGDEBUG("Execute errorred: %d", err);

Review comment:
       Sorry to be "that guy" about grammar, but errorred isn't really a word.  Should be "erred" or just avoid it altogether and use "failed".  Thx

##########
File path: cppcache/src/ExecutionImpl.cpp
##########
@@ -429,33 +429,20 @@ void ExecutionImpl::executeOnAllServers(const std::string& func,
   std::shared_ptr<CacheableString> exceptionPtr = nullptr;
   GfErrType err = tcrdm->sendRequestToAllServers(
       func.c_str(), getResult, timeout, m_args, m_rc, exceptionPtr);
-  if (exceptionPtr != nullptr && err != GF_NOERR) {
-    LOGDEBUG("Execute errorred: %d", err);
-    // throw FunctionExecutionException( "Execute: failed to execute function
-    // with server." );
-    if (err == GF_CACHESERVER_EXCEPTION) {
-      throw FunctionExecutionException(
-          "Execute: failed to execute function with server.");
-    } else {
-      throwExceptionIfError("Execute", err);
-    }
-  }
-
-  if (err == GF_AUTHENTICATION_FAILED_EXCEPTION ||
-      err == GF_NOT_AUTHORIZED_EXCEPTION ||
-      err == GF_AUTHENTICATION_REQUIRED_EXCEPTION) {
-    throwExceptionIfError("Execute", err);
-  }
 
   if (err != GF_NOERR) {
+    LOGDEBUG("Execute errorred: %d", err);

Review comment:
       Ugh, and it was even like this to begin with - yikes, sorry.




----------------------------------------------------------------
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] albertogpz commented on a change in pull request #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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



##########
File path: cppcache/src/ThinClientPoolDM.cpp
##########
@@ -2414,7 +2415,7 @@ GfErrType FunctionExecution::execute() {
   m_error = m_poolDM->sendRequestToEP(request, reply, m_ep);
   m_error = m_poolDM->handleEPError(m_ep, reply, m_error);
   if (m_error != GF_NOERR) {
-    if (m_error == GF_NOTCON || m_error == GF_IOERR) {
+    if (m_error == GF_NOTCON) {

Review comment:
       Removing that check is the way to signal the caller of this function (by means of the return in line 2428) that there has been an IO error so that the NotConnectedException is thrown to the client. With the original check, IO errors were ignored.




----------------------------------------------------------------
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 #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=h1) Report
   > Merging [#690](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=desc) (07eeb9f) into [develop](https://codecov.io/gh/apache/geode-native/commit/5c7d47d34c2b8a53874ec6f53e66c2290fd0427c?el=desc) (5c7d47d) will **decrease** coverage by `0.05%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/690/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #690      +/-   ##
   ===========================================
   - Coverage    74.12%   74.07%   -0.06%     
   ===========================================
     Files          644      644              
     Lines        51187    51184       -3     
   ===========================================
   - Hits         37942    37913      -29     
   - Misses       13245    13271      +26     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cppcache/src/CqQueryImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NxUXVlcnlJbXBsLmNwcA==) | `56.37% <0.00%> (ø)` | |
   | [cppcache/src/ExecutionImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0V4ZWN1dGlvbkltcGwuY3Bw) | `69.92% <28.57%> (+1.84%)` | :arrow_up: |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `77.36% <100.00%> (+0.87%)` | :arrow_up: |
   | [cppcache/src/ReadWriteLock.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1JlYWRXcml0ZUxvY2suY3Bw) | `62.50% <0.00%> (-37.50%)` | :arrow_down: |
   | [...tion-test/testThinClientRemoteQueryFailoverPdx.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFJlbW90ZVF1ZXJ5RmFpbG92ZXJQZHguY3Bw) | `85.48% <0.00%> (-4.04%)` | :arrow_down: |
   | [cppcache/src/TXCommitMessage.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RYQ29tbWl0TWVzc2FnZS5jcHA=) | `74.50% <0.00%> (-1.97%)` | :arrow_down: |
   | [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `55.11% <0.00%> (-1.55%)` | :arrow_down: |
   | [cppcache/src/ThinClientLocatorHelper.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRMb2NhdG9ySGVscGVyLmNwcA==) | `85.31% <0.00%> (-1.20%)` | :arrow_down: |
   | [...tegration-test/testThinClientRemoteRegionQuery.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFJlbW90ZVJlZ2lvblF1ZXJ5LmNwcA==) | `82.33% <0.00%> (-1.13%)` | :arrow_down: |
   | [cppcache/src/RemoteQuery.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1JlbW90ZVF1ZXJ5LmNwcA==) | `75.53% <0.00%> (-1.07%)` | :arrow_down: |
   | ... and [8 more](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/690?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/690?src=pr&el=footer). Last update [5c7d47d...07eeb9f](https://codecov.io/gh/apache/geode-native/pull/690?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 a change in pull request #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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



##########
File path: cppcache/integration/test/FunctionExecutionTest.cpp
##########
@@ -251,3 +253,79 @@ TEST(FunctionExecutionTest, OnServersWithReplicatedRegionsInPool) {
               resultList[i + ON_SERVERS_TEST_REGION_ENTRIES_SIZE / 2]);
   }
 }
+
+void executeTestFunctionOnLoopAndExpectNotConnectedException(
+    std::shared_ptr<apache::geode::client::Pool> pool) {
+  // Filter on odd keys
+  auto routingObj = CacheableVector::create();
+  for (int i = 0; i < ON_SERVERS_TEST_REGION_ENTRIES_SIZE; i++) {
+    if (i % 2 == 0) {
+      continue;
+    }
+    routingObj->push_back(CacheableString::create("KEY--" + std::to_string(i)));
+  }
+
+  auto execution = FunctionService::onServers(pool);
+
+  ASSERT_THROW(
+      {
+        while (true) {
+          // This call must eventually throw the NotConnectedException
+          auto rc =
+              execution.withArgs(routingObj).execute("MultiGetFunctionISlow");
+
+          auto executeFunctionResult = rc->getResult();
+
+          auto resultList = serverResultsToStrings(executeFunctionResult);
+
+          // Executed on 2 servers, we should have two sets of results
+          ASSERT_EQ(resultList.size(), ON_SERVERS_TEST_REGION_ENTRIES_SIZE);
+        }
+      },
+      apache::geode::client::NotConnectedException);
+}
+
+TEST(FunctionExecutionTest, OnServersOneServerGoesDown) {
+  Cluster cluster{
+      LocatorCount{1}, ServerCount{2},
+      CacheXMLFiles(
+          {std::string(getFrameworkString(FrameworkVariable::TestCacheXmlDir)) +
+               "/func_cacheserver1_pool.xml",
+           std::string(getFrameworkString(FrameworkVariable::TestCacheXmlDir)) +
+               "/func_cacheserver2_pool.xml"})};
+
+  cluster.start([&]() {
+    cluster.getGfsh()
+        .deploy()
+        .jar(getFrameworkString(FrameworkVariable::JavaObjectJarPath))
+        .execute();
+  });
+
+  auto cache = CacheFactory().set("log-level", "debug").create();
+  auto poolFactory = cache.getPoolManager().createFactory();
+
+  cluster.applyLocators(poolFactory);
+
+  auto pool =
+      poolFactory.setLoadConditioningInterval(std::chrono::milliseconds::zero())
+          .setIdleTimeout(std::chrono::milliseconds::zero())
+          .create("pool");
+
+  auto region = cache.createRegionFactory(RegionShortcut::PROXY)
+                    .setPoolName("pool")
+                    .create("partition_region");
+
+  for (int i = 0; i < ON_SERVERS_TEST_REGION_ENTRIES_SIZE; i++) {
+    region->put("KEY--" + std::to_string(i), "VALUE--" + std::to_string(i));
+  }
+
+  auto threadAux = std::make_shared<std::thread>(
+      executeTestFunctionOnLoopAndExpectNotConnectedException, pool);
+
+  // Sleep a bit to allow for some successful responses before the exception
+  sleep(5);

Review comment:
       This doesn't build on Windows:
   ```
   C:\nativeclient\cppcache\integration\test\FunctionExecutionTest.cpp(326): error C3861: 'sleep': identifier not found 
   ```
   Looks like we use `std::this_thread::sleep_for` in the rest of the code




----------------------------------------------------------------
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 #690: GEODE-8693: throw NotConnectedException if server goes down while exe…

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


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=h1) Report
   > Merging [#690](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=desc) (1ec096b) into [develop](https://codecov.io/gh/apache/geode-native/commit/5c7d47d34c2b8a53874ec6f53e66c2290fd0427c?el=desc) (5c7d47d) will **decrease** coverage by `0.02%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/690/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #690      +/-   ##
   ===========================================
   - Coverage    74.12%   74.09%   -0.03%     
   ===========================================
     Files          644      644              
     Lines        51187    51184       -3     
   ===========================================
   - Hits         37942    37926      -16     
   - Misses       13245    13258      +13     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/690?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cppcache/src/CqQueryImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NxUXVlcnlJbXBsLmNwcA==) | `56.37% <0.00%> (ø)` | |
   | [cppcache/src/ExecutionImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0V4ZWN1dGlvbkltcGwuY3Bw) | `69.92% <28.57%> (+1.84%)` | :arrow_up: |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `77.65% <100.00%> (+1.16%)` | :arrow_up: |
   | [cppcache/src/ReadWriteLock.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1JlYWRXcml0ZUxvY2suY3Bw) | `62.50% <0.00%> (-37.50%)` | :arrow_down: |
   | [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `54.27% <0.00%> (-2.39%)` | :arrow_down: |
   | [cppcache/src/TXCommitMessage.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RYQ29tbWl0TWVzc2FnZS5jcHA=) | `74.50% <0.00%> (-1.97%)` | :arrow_down: |
   | [...tegration-test/testThinClientRemoteRegionQuery.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFJlbW90ZVJlZ2lvblF1ZXJ5LmNwcA==) | `82.33% <0.00%> (-1.13%)` | :arrow_down: |
   | [cppcache/src/Log.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0xvZy5jcHA=) | `56.50% <0.00%> (-0.98%)` | :arrow_down: |
   | [cppcache/src/TcrConnection.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckNvbm5lY3Rpb24uY3Bw) | `72.79% <0.00%> (-0.95%)` | :arrow_down: |
   | [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `62.70% <0.00%> (-0.23%)` | :arrow_down: |
   | ... and [7 more](https://codecov.io/gh/apache/geode-native/pull/690/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/690?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/690?src=pr&el=footer). Last update [5c7d47d...1ec096b](https://codecov.io/gh/apache/geode-native/pull/690?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