[
https://issues.apache.org/jira/browse/GEODE-8543?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17276497#comment-17276497
]
ASF GitHub Bot commented on GEODE-8543:
---------------------------------------
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:
us...@infra.apache.org
> Client disconnects from server due to exception on other server
> ---------------------------------------------------------------
>
> Key: GEODE-8543
> URL: https://issues.apache.org/jira/browse/GEODE-8543
> Project: Geode
> Issue Type: Bug
> Components: native client
> Reporter: Alberto Bustamante Reyes
> Assignee: Jakov Varenina
> Priority: Major
> Labels: pull-request-available
>
> In ThinClientBaseDM::handleEPError when the client receives an exception from
> a given endpoint, it will mark that server dead if in the exception contains
> "java.lang.OutOfMemoryError",
> "org.apache.geode.distributed.ShutdownException" or
> "org.apache.geode.cache.CacheClosedException" ( in
> ThinClientBaseDM::unrecoverableServerError() )
> The problem is that the server causing the exception can be a different
> server than the endpoint the client is connected to. In that case,
> the client will wrongly close a connection to a working server.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
