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:
[email protected]
