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/05 14:00:53 UTC

[GitHub] [geode-native] albertogpz opened a new pull request #686: GEODE-8688: Fix flaky C++ native client integration tests

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


   The following integration test cases under
   integration/test (new integration tests)
   ar flaky (do not
   fail normally when run locally but fail very often
   when run in CI).
   
   - PartitionRegionOpsTest.getPartitionedRegionWithRedundancyServerGoesDownSingleHop
   - PartitionRegionOpsTest.putPartitionedRegionWithRedundancyServerGoesDownSingleHop
   
   There were two reasons that can make them fail.
   
   One of them is that sometimes the connections to the server have expired
   before the server is restarted and therefore, when traffic is sent
   to the restarted server, no errors are found. To fix this,
   the pool configuration for the test client
   has been changed so that connections do not expire.
   
   The other reason is that sometimes the error in the connection is
   found by the ping thread that is invoking the
   ThinClientPoolDM::sendRequestToEP() method and in this method,
   when the IO error or TIMEOUT error are encountered,
   the endpoint is not removed from the metadata (by means of the
   removeBucketServerLocation method).
   The code has been updated to remove the metadata also in this
   case.
   
   With these two changes, the test cases are not flaky anymore.


----------------------------------------------------------------
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 #686: GEODE-8688: Fix flaky C++ native client integration tests

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



##########
File path: cppcache/integration/test/PartitionRegionOpsTest.cpp
##########
@@ -144,9 +146,9 @@ void verifyMetadataWasRemovedAtFirstError() {
       }
     }
   }
-  ASSERT_TRUE((timeoutErrors == metadataRemovedDueToTimeout) &&
-              (ioErrors == metadataRemovedDueToIoErr) &&
-              (metadataRemovedDueToTimeout != metadataRemovedDueToIoErr));
+  ASSERT_EQ(timeoutErrors, metadataRemovedDueToTimeout);

Review comment:
       Alternatively, if you want to see the results of all three checks and fail if any of them fail you can use `EXPECT_*` macros. There are also more options for expecting and asserting in the `gmock` module. 




----------------------------------------------------------------
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 #686: GEODE-8688: Fix flaky C++ native client integration tests

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



##########
File path: cppcache/integration/test/PartitionRegionOpsTest.cpp
##########
@@ -144,9 +146,9 @@ void verifyMetadataWasRemovedAtFirstError() {
       }
     }
   }
-  ASSERT_TRUE((timeoutErrors == metadataRemovedDueToTimeout) &&
-              (ioErrors == metadataRemovedDueToIoErr) &&
-              (metadataRemovedDueToTimeout != metadataRemovedDueToIoErr));
+  ASSERT_EQ(timeoutErrors, metadataRemovedDueToTimeout);

Review comment:
       Thanks, I did not know.




----------------------------------------------------------------
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 #686: GEODE-8688: Fix flaky C++ native client integration tests

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



##########
File path: cppcache/integration/test/PartitionRegionOpsTest.cpp
##########
@@ -144,6 +146,10 @@ void verifyMetadataWasRemovedAtFirstError() {
       }
     }
   }
+  std::cout << "timeoutErrors: " << timeoutErrors << ", ioErrors: " << ioErrors
+            << ", metadataRemovedDueToTimeout: " << metadataRemovedDueToTimeout
+            << ", metadataRemovedDueToIoErr: " << metadataRemovedDueToIoErr
+            << std::endl;

Review comment:
       If you really want to see this output when running your test, I believe it's best to use std::cerr rather than cout.  This looks like leftover debugging trace stuff to me, tho.
   




----------------------------------------------------------------
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 #686: GEODE-8688: Fix flaky C++ native client integration tests

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



##########
File path: cppcache/integration/test/PartitionRegionOpsTest.cpp
##########
@@ -144,6 +146,10 @@ void verifyMetadataWasRemovedAtFirstError() {
       }
     }
   }
+  std::cout << "timeoutErrors: " << timeoutErrors << ", ioErrors: " << ioErrors
+            << ", metadataRemovedDueToTimeout: " << metadataRemovedDueToTimeout
+            << ", metadataRemovedDueToIoErr: " << metadataRemovedDueToIoErr
+            << std::endl;

Review comment:
       Tests should never output anything other than assertion failures. If more information is useful to log then it is useful to assert.




----------------------------------------------------------------
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 #686: GEODE-8688: Fix flaky C++ native client integration tests

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


   


----------------------------------------------------------------
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 #686: GEODE-8688: Fix flaky C++ native client integration tests

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


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/686?src=pr&el=h1) Report
   > Merging [#686](https://codecov.io/gh/apache/geode-native/pull/686?src=pr&el=desc) (ad2984f) into [develop](https://codecov.io/gh/apache/geode-native/commit/52c8c162afadee9d3da0ea41defbea8474f85e3f?el=desc) (52c8c16) will **decrease** coverage by `0.25%`.
   > The diff coverage is `92.85%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/686/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/686?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #686      +/-   ##
   ===========================================
   - Coverage    74.27%   74.01%   -0.26%     
   ===========================================
     Files          644      644              
     Lines        51187    51187              
   ===========================================
   - Hits         38017    37885     -132     
   - Misses       13170    13302     +132     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/686?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cppcache/src/ThinClientPoolDM.hpp](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uaHBw) | `88.70% <ø> (ø)` | |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `76.27% <90.00%> (+0.46%)` | :arrow_up: |
   | [cppcache/src/MapSegment.cpp](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL01hcFNlZ21lbnQuY3Bw) | `68.06% <100.00%> (+2.25%)` | :arrow_up: |
   | [cppcache/src/PutAllPartialResult.cpp](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1B1dEFsbFBhcnRpYWxSZXN1bHQuY3Bw) | `0.00% <0.00%> (-47.62%)` | :arrow_down: |
   | [cppcache/src/VersionedCacheableObjectPartList.hpp](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1ZlcnNpb25lZENhY2hlYWJsZU9iamVjdFBhcnRMaXN0LmhwcA==) | `52.70% <0.00%> (-27.03%)` | :arrow_down: |
   | [cppcache/src/CacheableObjectPartList.hpp](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NhY2hlYWJsZU9iamVjdFBhcnRMaXN0LmhwcA==) | `62.50% <0.00%> (-25.00%)` | :arrow_down: |
   | [cppcache/src/ReadWriteLock.cpp](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1JlYWRXcml0ZUxvY2suY3Bw) | `62.50% <0.00%> (-18.75%)` | :arrow_down: |
   | [cppcache/src/ThinClientRegion.cpp](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWdpb24uY3Bw) | `56.04% <0.00%> (-4.67%)` | :arrow_down: |
   | [cppcache/src/PutAllPartialResult.hpp](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1B1dEFsbFBhcnRpYWxSZXN1bHQuaHBw) | `0.00% <0.00%> (-4.55%)` | :arrow_down: |
   | [...tion-test/testThinClientRemoteQueryFailoverPdx.cpp](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFJlbW90ZVF1ZXJ5RmFpbG92ZXJQZHguY3Bw) | `85.48% <0.00%> (-4.04%)` | :arrow_down: |
   | ... and [12 more](https://codecov.io/gh/apache/geode-native/pull/686/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/686?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/686?src=pr&el=footer). Last update [0791032...ad2984f](https://codecov.io/gh/apache/geode-native/pull/686?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