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/09/30 03:20:40 UTC
[GitHub] [geode-native] gaussianrecurrence opened a new pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
gaussianrecurrence opened a new pull request #660:
URL: https://github.com/apache/geode-native/pull/660
- ThinClientLocatorHelper uses a mutex called m_locatorLock, which is
used in the entire scope of all the public member functions for this
class.
- Problem with that is all of those functions perform some network
communication and in the event of networking issues, this will result
in having all the application threads inter-locked.
- Also, the only purpose of this mutex is to avoid that the locators
list is read while it's being updated.
- Given all of the above, the class has been refactored, so every time
the locators list has to be accessed, a copy of it is created, being
that copy created only while owning the mutex.
- And for the case of the update, a new locators list is composed and
its only at the end of the update function where the mutex is locked
and the locators list swapped by the new one.
- This whole change ensures that the time the mutex is in use is
minimal while the functionality remains intact.
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-710780091
Hi
Sorry to go into the topic again, but @mreddington @pdxcodemonkey or @pivotal-jbarrett did you had the chance to look into it?
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497880165
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -74,10 +74,15 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
m_sniProxyHost(sniProxyHost),
m_sniProxyPort(sniProxyPort) {
for (auto&& locatorAddress : locatorAddresses) {
- m_locHostPort.emplace_back(locatorAddress);
+ m_Locators.emplace_back(locatorAddress);
}
}
+std::vector<ServerLocation> ThinClientLocatorHelper::getLocators() const {
Review comment:
As for the random_shuffle, indeed, I agree π
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-721399787
Ok @pdxcodemonkey and @pivotal-jbarrett I think I came up with an integration test for this change. It works as follows:
* Set 5s connect-timeout.
* Deploy 1 locator.
* Add 1 non-existent locator to the pool.
* Send 64 concurrent requests to get the server list.
* By probability if there are no interlocks ~50%+-E% of the requests should
have completed in a brief period of time.
* Note that E% is the error allowance percentage, which is set to 10%.
* Therefore a 10 seconds timeout is set in order to reach the request
completed treshold.
* Test passes if the number of completed requests before the timeout was
greater or equal to the thresholds, otherwise it fails.
It does not cover everything, but I'd would appreciate your feedback in order to see If I am in the right direction. Thanks!
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497868084
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -74,10 +74,15 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
m_sniProxyHost(sniProxyHost),
m_sniProxyPort(sniProxyPort) {
for (auto&& locatorAddress : locatorAddresses) {
- m_locHostPort.emplace_back(locatorAddress);
+ m_Locators.emplace_back(locatorAddress);
}
}
+std::vector<ServerLocation> ThinClientLocatorHelper::getLocators() const {
Review comment:
Regarding the lack of necessity for this member function to be const-qualified, this is totally true, but on the other hand, all but updateLocators member function could be const-qualified as they don't modify the object state.
My opinion on this is make everything const-qualified unless strictly necessary, this way you don't have problems to integrate with other implementations.
An example of this is:
We had this project in which we we trying to do PdxInstance deserialization, and the method deseralize was const-qualified and given, several methods in PdxInstance weren't const-qualified, we needed to use the const_cast nasty trick :S
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r500563207
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -174,30 +176,25 @@ GfErrType ThinClientLocatorHelper::getEndpointForNewCallBackConn(
ClientProxyMembershipID& memId, std::list<ServerLocation>& outEndpoint,
std::string&, int redundancy, const std::set<ServerLocation>& exclEndPts,
const std::string& serverGrp) {
- std::lock_guard<decltype(m_locatorLock)> guard(m_locatorLock);
auto& sysProps = m_poolDM->getConnectionManager()
.getCacheImpl()
->getDistributedSystem()
.getSystemProperties();
- int locatorsRetry = 3;
- if (m_poolDM) {
- int poolRetry = m_poolDM->getRetryAttempts();
- locatorsRetry = poolRetry <= 0 ? locatorsRetry : poolRetry;
- }
+
+ auto poolRetry = m_poolDM->getRetryAttempts();
+ auto locatorsRetry = poolRetry <= 0 ? 3 : poolRetry;
+
LOGFINER(
"ThinClientLocatorHelper::getEndpointForNewCallBackConn locatorsRetry = "
"%d ",
locatorsRetry);
- for (unsigned attempts = 0;
- attempts <
- (m_locHostPort.size() == 1 ? locatorsRetry : m_locHostPort.size());
- attempts++) {
- ServerLocation loc;
- if (m_locHostPort.size() == 1) {
- loc = m_locHostPort[0];
- } else {
- loc = m_locHostPort[attempts];
- }
+
+ auto locators = getLocators();
+ auto locatorsSize = locators.size();
+ auto maxAttempts = locatorsSize == 1 ? locatorsRetry : locatorsSize;
+
+ for (auto attempts = 0ULL; attempts < maxAttempts; ++attempts) {
+ const auto& loc = locatorsSize == 1 ? locators[0] : locators[attempts];
Review comment:
Perfect, then I'll resolve this conversation :)
----------------------------------------------------------------
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] mreddington commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497814165
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -60,20 +63,23 @@ class ThinClientLocatorHelper {
std::vector<std::shared_ptr<ServerLocation> >& servers,
const std::string& serverGrp);
int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ return static_cast<int32_t>(m_Locators.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
+ std::vector<ServerLocation> getLocators() const;
Connector* createConnection(Connector*& conn, const char* hostname,
int32_t port,
std::chrono::microseconds waitSeconds,
int32_t maxBuffSizePool = 0);
- std::mutex m_locatorLock;
- std::vector<ServerLocation> m_locHostPort;
+
+ /**
+ * Data members
+ */
+ mutable std::mutex m_locatorLock;
Review comment:
There is no need to make this mutable as no method that uses the mutex is const except for getLocators.
----------------------------------------------------------------
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] mreddington commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497817228
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -62,7 +62,7 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
const ThinClientPoolDM* poolDM)
: m_poolDM(poolDM) {
for (auto&& locatorAddress : locatorAddresses) {
Review comment:
Prefer vector copy constructor or iterator range constructor. Make `ServerLocation(std::string)` non-explicit if you have to (it seems to be some confused logic that it's explicit in the first place).
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-702760316
I'll think about your idea today and get back to you. My first instinct is that it's a much larger change than even the initial one, however, so it may be beyond the scope of what I'm comfortable with, absent a _lot_ more testing etc.
In the meantime, your last commit broke our Windows build, maybe due to some platform difference in boost(?). Here's the error output:
```
ThinClientPoolStickyDM.cpp
C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(73): error C2039: 'shared_lock': is not a member of 'boost' [C:\build\cppcache\static\apache-geode-static.vcxproj]
C:\build\dependencies\boost\RelWithDebInfo\include\boost/thread/shared_mutex.hpp(36): note: see declaration of 'boost'
C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(73): error C2065: 'shared_lock': undeclared identifier [C:\build\cppcache\static\apache-geode-static.vcxproj]
C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(73): error C2226: syntax error: unexpected type 'boost::shared_mutex' [C:\build\cppcache\static\apache-geode-static.vcxproj]
C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(73): error C2143: syntax error: missing ';' before '{' [C:\build\cppcache\static\apache-geode-static.vcxproj]
C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(73): error C2143: syntax error: missing ';' before '}' [C:\build\cppcache\static\apache-geode-static.vcxproj]
C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(303): error C2039: 'unique_lock': is not a member of 'boost' [C:\build\cppcache\static\apache-geode-static.vcxproj]
C:\build\dependencies\boost\RelWithDebInfo\include\boost/thread/shared_mutex.hpp(36): note: see declaration of 'boost'
C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(303): error C2065: 'unique_lock': undeclared identifier [C:\build\cppcache\static\apache-geode-static.vcxproj]
C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(303): error C2226: syntax error: unexpected type 'boost::shared_mutex' [C:\build\cppcache\static\apache-geode-static.vcxproj]
```
I'll try a build on a Windows instance here, and let you know if I find out what's up before you. Thanks!
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-703156801
Hi,
Ok... got it finally. After fighting 1-2 days with VS and the old testing framework I located the problem.
Problem has to do with move semantics and ConnectionWrapper. Thing is for some reason I forgot how to program and I implemented the movement of the conn_ attr just as conn_(std::move(other.conn_)) and whenever the original object conn_ pointer is not nullptr, therefore the pointer is destroyed, leaving the second object pointing to an invalid.
Thing is, it seems GCC implementation of the standard is different than MSVC's and it avoids calling to the move constructor and that's why test were passing in Linux.
Will upload a fix ASAP.
BR,
Mario.
----------------------------------------------------------------
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] mreddington commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r500561294
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -174,30 +176,25 @@ GfErrType ThinClientLocatorHelper::getEndpointForNewCallBackConn(
ClientProxyMembershipID& memId, std::list<ServerLocation>& outEndpoint,
std::string&, int redundancy, const std::set<ServerLocation>& exclEndPts,
const std::string& serverGrp) {
- std::lock_guard<decltype(m_locatorLock)> guard(m_locatorLock);
auto& sysProps = m_poolDM->getConnectionManager()
.getCacheImpl()
->getDistributedSystem()
.getSystemProperties();
- int locatorsRetry = 3;
- if (m_poolDM) {
- int poolRetry = m_poolDM->getRetryAttempts();
- locatorsRetry = poolRetry <= 0 ? locatorsRetry : poolRetry;
- }
+
+ auto poolRetry = m_poolDM->getRetryAttempts();
+ auto locatorsRetry = poolRetry <= 0 ? 3 : poolRetry;
+
LOGFINER(
"ThinClientLocatorHelper::getEndpointForNewCallBackConn locatorsRetry = "
"%d ",
locatorsRetry);
- for (unsigned attempts = 0;
- attempts <
- (m_locHostPort.size() == 1 ? locatorsRetry : m_locHostPort.size());
- attempts++) {
- ServerLocation loc;
- if (m_locHostPort.size() == 1) {
- loc = m_locHostPort[0];
- } else {
- loc = m_locHostPort[attempts];
- }
+
+ auto locators = getLocators();
+ auto locatorsSize = locators.size();
+ auto maxAttempts = locatorsSize == 1 ? locatorsRetry : locatorsSize;
+
+ for (auto attempts = 0ULL; attempts < maxAttempts; ++attempts) {
+ const auto& loc = locatorsSize == 1 ? locators[0] : locators[attempts];
Review comment:
I'm not opposed to changing the business logic, since A) this business logic is not part of any formal specification, we make no promises here and are free to do what we want, and B) this code contains obvious bugs.
That said, so long as the bug is addressed, whatever solution you like. Your suggestion is just fine.
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-707916432
@gaussianrecurrence Is this ready for re-review?
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-703724456
Windows build still hitting an issue:
```
C:\nativeclient\cppcache\src\ThinClientPoolDM.cpp(572): error C2220: warning treated as error - no 'object' file generated [C:\build\cppcache\static\apache-geode-static.vcxproj]
C:\nativeclient\cppcache\src\ThinClientPoolDM.cpp(572): warning C4267: 'argument': conversion from 'size_t' to 'int32_t', possible loss of data [C:\build\cppcache\static\apache-geode-static.vcxproj]
```
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497865566
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -60,20 +63,23 @@ class ThinClientLocatorHelper {
std::vector<std::shared_ptr<ServerLocation> >& servers,
const std::string& serverGrp);
int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ return static_cast<int32_t>(m_Locators.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
+ std::vector<ServerLocation> getLocators() const;
Connector* createConnection(Connector*& conn, const char* hostname,
int32_t port,
std::chrono::microseconds waitSeconds,
int32_t maxBuffSizePool = 0);
- std::mutex m_locatorLock;
- std::vector<ServerLocation> m_locHostPort;
+
+ /**
+ * Data members
+ */
+ mutable std::mutex m_locatorLock;
Review comment:
Please read my answer to @pdxcodemonkey :)
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-704262279
> If you push the build fix to your branch, my pipeline will run build/test for all platforms automatically...
Oh that's nice. On either case I completed the execution of the tests.
They are all passing except for this new integration test:
BasicIPv6Test.queryResultForRange
Thing is the problem seems ip6-locahost does not exist in my host. I modified it so it uses ::1 as binding address, just to see if that was the problem but it failed due to ACE not being able to find the host. Which leds me to think.
Shouldn't be IPV6 tests disabled if WITH_IPV6 CMake option is not enabled?
Or should I always compile with IPV6 in order to run the tests?
Summaziring here: "In theory all test should pass except for the IPV6 one"
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497846453
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -174,30 +176,25 @@ GfErrType ThinClientLocatorHelper::getEndpointForNewCallBackConn(
ClientProxyMembershipID& memId, std::list<ServerLocation>& outEndpoint,
std::string&, int redundancy, const std::set<ServerLocation>& exclEndPts,
const std::string& serverGrp) {
- std::lock_guard<decltype(m_locatorLock)> guard(m_locatorLock);
auto& sysProps = m_poolDM->getConnectionManager()
.getCacheImpl()
->getDistributedSystem()
.getSystemProperties();
- int locatorsRetry = 3;
- if (m_poolDM) {
- int poolRetry = m_poolDM->getRetryAttempts();
- locatorsRetry = poolRetry <= 0 ? locatorsRetry : poolRetry;
- }
+
+ auto poolRetry = m_poolDM->getRetryAttempts();
+ auto locatorsRetry = poolRetry <= 0 ? 3 : poolRetry;
+
LOGFINER(
"ThinClientLocatorHelper::getEndpointForNewCallBackConn locatorsRetry = "
"%d ",
locatorsRetry);
- for (unsigned attempts = 0;
- attempts <
- (m_locHostPort.size() == 1 ? locatorsRetry : m_locHostPort.size());
- attempts++) {
- ServerLocation loc;
- if (m_locHostPort.size() == 1) {
- loc = m_locHostPort[0];
- } else {
- loc = m_locHostPort[attempts];
- }
+
+ auto locators = getLocators();
+ auto locatorsSize = locators.size();
+ auto maxAttempts = locatorsSize == 1 ? locatorsRetry : locatorsSize;
+
+ for (auto attempts = 0ULL; attempts < maxAttempts; ++attempts) {
+ const auto& loc = locatorsSize == 1 ? locators[0] : locators[attempts];
Review comment:
You've totally got a point. Much more clear that way π
----------------------------------------------------------------
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] mreddington commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497826366
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -174,30 +176,25 @@ GfErrType ThinClientLocatorHelper::getEndpointForNewCallBackConn(
ClientProxyMembershipID& memId, std::list<ServerLocation>& outEndpoint,
std::string&, int redundancy, const std::set<ServerLocation>& exclEndPts,
const std::string& serverGrp) {
- std::lock_guard<decltype(m_locatorLock)> guard(m_locatorLock);
auto& sysProps = m_poolDM->getConnectionManager()
.getCacheImpl()
->getDistributedSystem()
.getSystemProperties();
- int locatorsRetry = 3;
- if (m_poolDM) {
- int poolRetry = m_poolDM->getRetryAttempts();
- locatorsRetry = poolRetry <= 0 ? locatorsRetry : poolRetry;
- }
+
+ auto poolRetry = m_poolDM->getRetryAttempts();
+ auto locatorsRetry = poolRetry <= 0 ? 3 : poolRetry;
+
LOGFINER(
"ThinClientLocatorHelper::getEndpointForNewCallBackConn locatorsRetry = "
"%d ",
locatorsRetry);
- for (unsigned attempts = 0;
- attempts <
- (m_locHostPort.size() == 1 ? locatorsRetry : m_locHostPort.size());
- attempts++) {
- ServerLocation loc;
- if (m_locHostPort.size() == 1) {
- loc = m_locHostPort[0];
- } else {
- loc = m_locHostPort[attempts];
- }
+
+ auto locators = getLocators();
+ auto locatorsSize = locators.size();
+ auto maxAttempts = locatorsSize == 1 ? locatorsRetry : locatorsSize;
+
+ for (auto attempts = 0ULL; attempts < maxAttempts; ++attempts) {
+ const auto& loc = locatorsSize == 1 ? locators[0] : locators[attempts];
Review comment:
This logic is flawed. What if maxAttempts is 3 and locatorsSize is 2? What this code says is if there is 1 locator, all attempts are on that locator, otherwise, make one attempt per each locator. I suggest - unless you have a better idea, the logic should be per each attempt, try all locators until one connection is successful. By moving the `random_shuffle` into `getLocators`, the order will be randomized every time, which the NC doesn't make any guarantee about locator order, so this seems ideal.
I recommend a helper function like `try_n(functor, count)`. The functor should return some GfErrType value. The utility function will loop until success is indicated by the functor or it runs out of attempts, and either throws an exception upon failure or returns that GfErrType - however you want to handle the fail case...
Write a function `connect_to_a_locator` contains the loop logic over the locators and the connect attempts and returns GfErrType, which you can bind, or it can return a lambda that does the same thing. Suggestion #1:
GfErrType ThinClientLocatorHelper::connect_to_a_locator() {
/* business logic */
}
Suggestion #2:
auto ThinClientLocatorHelper::connect_to_a_locator() {
return [&]() -> GfErrType {
/* business logic */
};
}
The code would then look like:
try_n(::std::bind(ThinClientLocatorHelper::connect_to_a_locator, this), maxAttempts);
Or:
try_n(connect_to_a_locator(), maxAttempts);
Maybe checking a return type, maybe in a try/catch block.
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-703854698
If you push the build fix to your branch, my pipeline will run build/test for all platforms automatically...
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-702875371
@gaussianrecurrence Yes we have a CI pipeline for all the builds/tests, we're working out how to donate and make it public. In the meantime, we run all incoming PR branches through it to avoid unpleasant surprises.
I'm now seeing a ton of test failures on Windows:
18% tests passed, 86 tests failed out of 105
Label Time Summary:
FLAKY = 0.00 sec*proc (42 tests)
OMITTED = 0.00 sec*proc (20 tests)
QUICK = 60.32 sec*proc (15 tests)
STABLE = 13222.11 sec*proc (105 tests)
Total Test time (real) = 3604.23 sec
The following tests did not run:
7 - testFwPerf (Disabled)
11 - testOverflowPutGetSqLite (Disabled)
22 - testThinClientAfterRegionLive (Disabled)
25 - testThinClientCacheables (Disabled)
31 - testThinClientCq (Disabled)
33 - testThinClientCqDurable (Disabled)
34 - testThinClientCqFailover (Disabled)
35 - testThinClientCqHAFailover (Disabled)
44 - testThinClientDurableConnect (Disabled)
47 - testThinClientDurableDisconnectNormal (Disabled)
48 - testThinClientDurableDisconnectTimeout (Disabled)
49 - testThinClientDurableFailoverClientClosedNoRedundancy (Disabled)
51 - testThinClientDurableFailoverClientNotClosedRedundancy (Disabled)
54 - testThinClientDurableKeepAliveFalseTimeout (Disabled)
55 - testThinClientDurableKeepAliveTrueNormal (Disabled)
57 - testThinClientDurableReconnect (Disabled)
58 - testThinClientFailover (Disabled)
59 - testThinClientFailover2 (Disabled)
61 - testThinClientFailoverInterest (Disabled)
62 - testThinClientFailoverInterest2 (Disabled)
64 - testThinClientFailoverRegex (Disabled)
65 - testThinClientFixedPartitionResolver (Disabled)
66 - testThinClientGatewayTest (Disabled)
68 - testThinClientHADistOps (Disabled)
69 - testThinClientHAEventIDMap (Disabled)
70 - testThinClientHAFailover (Disabled)
71 - testThinClientHAFailoverRegex (Disabled)
72 - testThinClientHAMixedRedundancy (Disabled)
74 - testThinClientHAQueryFailover (Disabled)
86 - testThinClientLRUExpiration (Disabled)
87 - testThinClientLargePutAllWithCallBackArg (Disabled)
95 - testThinClientLocatorFailover (Disabled)
96 - testThinClientMultiDS (Disabled)
100 - testThinClientPRPutAllFailover (Disabled)
101 - testThinClientPRSingleHop (Disabled)
103 - testThinClientPartitionResolver (Disabled)
104 - testThinClientPdxDeltaWithNotification (Disabled)
109 - testThinClientPdxTests (Disabled)
110 - testThinClientPoolAttrTest (Disabled)
114 - testThinClientPoolLocator (Disabled)
115 - testThinClientPoolRedundancy (Disabled)
117 - testThinClientPoolServer (Disabled)
118 - testThinClientPutAll (Disabled)
119 - testThinClientPutAllPRSingleHop (Disabled)
122 - testThinClientPutAllWithCallBackArgWithoutConcurrency (Disabled)
124 - testThinClientPutWithDelta (Disabled)
135 - testThinClientRemoteQueryTimeout (Disabled)
140 - testThinClientRemoveOps (Disabled)
141 - testThinClientSecurityAuthentication (Disabled)
142 - testThinClientSecurityAuthenticationMU (Disabled)
144 - testThinClientSecurityAuthorization (Disabled)
145 - testThinClientSecurityAuthorizationMU (Disabled)
146 - testThinClientSecurityCQAuthorizationMU (Disabled)
147 - testThinClientSecurityDurableCQAuthorizationMU (Disabled)
149 - testThinClientSecurityPostAuthorization (Disabled)
150 - testThinClientTXFailover (Disabled)
151 - testThinClientTicket303 (Disabled)
152 - testThinClientTicket304 (Disabled)
154 - testThinClientTracking (Disabled)
157 - testThinClientTransactionsXA (Disabled)
162 - testThinClientWriterException (Disabled)
163 - testTimedSemaphore (Disabled)
The following tests FAILED:
1 - testCacheless (Timeout)
12 - testPdxMetadataCheckTest (Failed)
14 - testRegionAccessThreadSafe (Failed)
18 - testSerialization (Failed)
23 - testThinClientBigValue (Failed)
24 - testThinClientCacheableStringArray (Failed)
26 - testThinClientCacheablesLimits (Failed)
27 - testThinClientCallbackArg (Failed)
28 - testThinClientClearRegion (Failed)
29 - testThinClientConflation (Failed)
30 - testThinClientContainsKeyOnServer (Failed)
32 - testThinClientCqDelta (Failed)
36 - testThinClientCqIR (Failed)
37 - testThinClientDeltaWithNotification (Failed)
38 - testThinClientDisconnectionListioner (Failed)
39 - testThinClientDistOps2 (Failed)
40 - testThinClientDistOpsDontUpdateLocatorList (Failed)
41 - testThinClientDistOpsNotSticky (Failed)
42 - testThinClientDistOpsSticky (Failed)
43 - testThinClientDistOpsUpdateLocatorList (Failed)
45 - testThinClientDurableCrashNormal (Failed)
46 - testThinClientDurableCrashTimeout (Failed)
50 - testThinClientDurableFailoverClientClosedRedundancy (Failed)
52 - testThinClientDurableInterest (Failed)
53 - testThinClientDurableKeepAliveFalseNormal (Failed)
56 - testThinClientDurableKeepAliveTrueTimeout (Failed)
60 - testThinClientFailover3 (Failed)
63 - testThinClientFailoverInterestAllWithCache (Failed)
67 - testThinClientGetInterests (Failed)
73 - testThinClientHAPeriodicAck (Failed)
75 - testThinClientHeapLRU (Failed)
76 - testThinClientIntResPolKeysInv (Failed)
77 - testThinClientInterest1 (Failed)
78 - testThinClientInterest1Cacheless (Failed)
79 - testThinClientInterest1_Bug1001 (Failed)
80 - testThinClientInterest2Pooled (Failed)
81 - testThinClientInterest3 (Failed)
82 - testThinClientInterest3Cacheless (Failed)
83 - testThinClientInterestList (Failed)
84 - testThinClientInterestList2 (Failed)
85 - testThinClientInterestNotify (Timeout)
88 - testThinClientListenerCallbackArgTest (Failed)
89 - testThinClientListenerEvents (Failed)
90 - testThinClientListenerInit (Failed)
91 - testThinClientListenerWriterWithSubscription (Failed)
92 - testThinClientListenerWriterWithoutSubscription (Failed)
94 - testThinClientLocator (Failed)
97 - testThinClientMultipleCaches (Failed)
98 - testThinClientNotification (Failed)
99 - testThinClientNotificationWithDeltaWithoutcache (Failed)
102 - testThinClientPRSingleHopServerGroup (Failed)
105 - testThinClientPdxEnum (Failed)
106 - testThinClientPdxInstance (Failed)
107 - testThinClientPdxSerializer (Failed)
108 - testThinClientPdxSerializerForJava (Failed)
111 - testThinClientPoolExecuteFunctionThrowsException (Failed)
112 - testThinClientPoolExecuteHAFunction (Failed)
113 - testThinClientPoolExecuteHAFunctionPrSHOP (Failed)
116 - testThinClientPoolRegInterest (Failed)
120 - testThinClientPutAllTimeout (Failed)
121 - testThinClientPutAllWithCallBackArgWithConcurrency (Failed)
123 - testThinClientPutGetAll (Failed)
125 - testThinClientRIwithlocalRegionDestroy (Failed)
126 - testThinClientRegex (Failed)
127 - testThinClientRegex2 (Failed)
128 - testThinClientRegex3 (Failed)
129 - testThinClientRegionQueryDifferentServerConfigs (Failed)
130 - testThinClientRegionQueryExclusiveness (Failed)
131 - testThinClientRemoteQueryFailover (Failed)
132 - testThinClientRemoteQueryFailoverPdx (Failed)
133 - testThinClientRemoteQueryRS (Failed)
134 - testThinClientRemoteQuerySS (Failed)
136 - testThinClientRemoteRegionQuery (Failed)
137 - testThinClientRemoveAll (Failed)
139 - testThinClientRemoveAllSequence (Failed)
143 - testThinClientSecurityAuthenticationSetAuthInitialize (Failed)
148 - testThinClientSecurityMultiUserTest (Failed)
153 - testThinClientTicket317 (Failed)
155 - testThinClientTransactionsWithSticky (Failed)
156 - testThinClientTransactionsWithoutSticky (Failed)
158 - testThinClientVersionedOps (Failed)
159 - testThinClientVersionedOpsPartitionPersistent (Failed)
160 - testThinClientVersionedOpsReplicate (Failed)
161 - testThinClientVersionedOpsReplicatePersistent (Failed)
165 - testXmlCacheCreationWithPools (Timeout)
167 - testXmlCacheInitialization (Timeout)
Errors while running CTest
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497891939
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -62,7 +62,7 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
const ThinClientPoolDM* poolDM)
: m_poolDM(poolDM) {
for (auto&& locatorAddress : locatorAddresses) {
Review comment:
Got it π
----------------------------------------------------------------
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] lgtm-com[bot] commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-703275195
This pull request **introduces 2 alerts** when merging 66c5679134de157db62c966be8ab3960b1973782 into 2e64e74e3b29c9813776c80b34b84606ec7b8274 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-09f33402e6c86fb562c26a7c9e1f1d1cf418fe6a)
**new alerts:**
* 2 for Wrong type of arguments to formatting function
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497871785
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -60,20 +63,23 @@ class ThinClientLocatorHelper {
std::vector<std::shared_ptr<ServerLocation> >& servers,
const std::string& serverGrp);
int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ return static_cast<int32_t>(m_Locators.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
+ std::vector<ServerLocation> getLocators() const;
Connector* createConnection(Connector*& conn, const char* hostname,
int32_t port,
std::chrono::microseconds waitSeconds,
int32_t maxBuffSizePool = 0);
- std::mutex m_locatorLock;
- std::vector<ServerLocation> m_locHostPort;
+
+ /**
+ * Data members
+ */
+ mutable std::mutex m_locatorLock;
Review comment:
Also, after seeing your comment it came out to my mind that this very mutex is a good candidate to be a shared_mutex, don't you think? I mean, there are lots of readers, and just a single writer :)
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497892014
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -74,10 +74,15 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
m_sniProxyHost(sniProxyHost),
m_sniProxyPort(sniProxyPort) {
for (auto&& locatorAddress : locatorAddresses) {
Review comment:
Got it π
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497908712
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -174,30 +176,25 @@ GfErrType ThinClientLocatorHelper::getEndpointForNewCallBackConn(
ClientProxyMembershipID& memId, std::list<ServerLocation>& outEndpoint,
std::string&, int redundancy, const std::set<ServerLocation>& exclEndPts,
const std::string& serverGrp) {
- std::lock_guard<decltype(m_locatorLock)> guard(m_locatorLock);
auto& sysProps = m_poolDM->getConnectionManager()
.getCacheImpl()
->getDistributedSystem()
.getSystemProperties();
- int locatorsRetry = 3;
- if (m_poolDM) {
- int poolRetry = m_poolDM->getRetryAttempts();
- locatorsRetry = poolRetry <= 0 ? locatorsRetry : poolRetry;
- }
+
+ auto poolRetry = m_poolDM->getRetryAttempts();
+ auto locatorsRetry = poolRetry <= 0 ? 3 : poolRetry;
+
LOGFINER(
"ThinClientLocatorHelper::getEndpointForNewCallBackConn locatorsRetry = "
"%d ",
locatorsRetry);
- for (unsigned attempts = 0;
- attempts <
- (m_locHostPort.size() == 1 ? locatorsRetry : m_locHostPort.size());
- attempts++) {
- ServerLocation loc;
- if (m_locHostPort.size() == 1) {
- loc = m_locHostPort[0];
- } else {
- loc = m_locHostPort[attempts];
- }
+
+ auto locators = getLocators();
+ auto locatorsSize = locators.size();
+ auto maxAttempts = locatorsSize == 1 ? locatorsRetry : locatorsSize;
+
+ for (auto attempts = 0ULL; attempts < maxAttempts; ++attempts) {
+ const auto& loc = locatorsSize == 1 ? locators[0] : locators[attempts];
Review comment:
I don't like either the way code is written for connections attemps. Problem with your approach is that it would change the business logic, meaning it might have some impact in terms of robustness.
The way I see it, connections attemps towards locators should work as follows:
- Call getLocators (considering vector is shuffled each call), returning the list of locators.
- Treat the list of locators as a circular buffer and make locatorsRetry connections attempts meaning, the i-th connection attempt will be made to the (i mod locators.size()) locator.
How about this? :)
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-707918209
> @gaussianrecurrence Is this ready for re-review?
Sure. Only thing left in here is the testing part, for which I would really appreciate any ideas.
As I told you we could even make a previous PR in which we add the necessary scaffolding to test this.
Or maybe there is another way I did not come up with, which is much simpler.
Thanks for putting some time on this :)
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497675027
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -62,7 +62,7 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
const ThinClientPoolDM* poolDM)
: m_poolDM(poolDM) {
for (auto&& locatorAddress : locatorAddresses) {
- m_locHostPort.emplace_back(locatorAddress);
+ m_Locators.emplace_back(locatorAddress);
Review comment:
If you're already renaming a member variable, it'd be great to make it conform to our preferred style, basically Google conventions. This would just be `locators_`, with a trailing underscore. Our rule of thumb for small refactors like this is to go ahead and do them as long as you're already editing the file in your PR.
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -174,30 +176,25 @@ GfErrType ThinClientLocatorHelper::getEndpointForNewCallBackConn(
ClientProxyMembershipID& memId, std::list<ServerLocation>& outEndpoint,
std::string&, int redundancy, const std::set<ServerLocation>& exclEndPts,
const std::string& serverGrp) {
- std::lock_guard<decltype(m_locatorLock)> guard(m_locatorLock);
auto& sysProps = m_poolDM->getConnectionManager()
.getCacheImpl()
->getDistributedSystem()
.getSystemProperties();
- int locatorsRetry = 3;
- if (m_poolDM) {
- int poolRetry = m_poolDM->getRetryAttempts();
- locatorsRetry = poolRetry <= 0 ? locatorsRetry : poolRetry;
- }
+
+ auto poolRetry = m_poolDM->getRetryAttempts();
+ auto locatorsRetry = poolRetry <= 0 ? 3 : poolRetry;
+
LOGFINER(
"ThinClientLocatorHelper::getEndpointForNewCallBackConn locatorsRetry = "
"%d ",
locatorsRetry);
- for (unsigned attempts = 0;
- attempts <
- (m_locHostPort.size() == 1 ? locatorsRetry : m_locHostPort.size());
- attempts++) {
- ServerLocation loc;
- if (m_locHostPort.size() == 1) {
- loc = m_locHostPort[0];
- } else {
- loc = m_locHostPort[attempts];
- }
+
+ auto locators = getLocators();
+ auto locatorsSize = locators.size();
+ auto maxAttempts = locatorsSize == 1 ? locatorsRetry : locatorsSize;
+
+ for (auto attempts = 0ULL; attempts < maxAttempts; ++attempts) {
+ const auto& loc = locatorsSize == 1 ? locators[0] : locators[attempts];
Review comment:
A local variable that shadows `locators.size()` doesn't really clarify things much. How about a boolean that represents the test you're doing? Something like:
```
auto locators = getLocators();
auto singleLocator = locators.size() == 1;
auto maxAttempts = singleLocator ? locatorsRetry : locators.size();
for (auto attempts = 0; attempts < maxAttempts; ++attempts) {
const auto& loc = singleLocator ? locators[0] : locators[attempts];
```
I'm not sure whether I prefer this or not, let me know what you think....
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -357,22 +349,20 @@ GfErrType ThinClientLocatorHelper::getEndpointForNewFwdConn(
GfErrType ThinClientLocatorHelper::updateLocators(
const std::string& serverGrp) {
- std::lock_guard<decltype(m_locatorLock)> guard(m_locatorLock);
auto& sysProps = m_poolDM->getConnectionManager()
.getCacheImpl()
->getDistributedSystem()
.getSystemProperties();
- for (size_t attempts = 0; attempts < m_locHostPort.size(); attempts++) {
- auto&& serLoc = m_locHostPort[attempts];
+ auto locators = getLocators();
+ for (const auto& loc : locators) {
Review comment:
This is so much better - thanks!
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -60,20 +63,23 @@ class ThinClientLocatorHelper {
std::vector<std::shared_ptr<ServerLocation> >& servers,
const std::string& serverGrp);
int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ return static_cast<int32_t>(m_Locators.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
+ std::vector<ServerLocation> getLocators() const;
Connector* createConnection(Connector*& conn, const char* hostname,
int32_t port,
std::chrono::microseconds waitSeconds,
int32_t maxBuffSizePool = 0);
- std::mutex m_locatorLock;
- std::vector<ServerLocation> m_locHostPort;
+
+ /**
+ * Data members
+ */
+ mutable std::mutex m_locatorLock;
Review comment:
The `mutable` keyword is usually a "code smell" - is this just needed in order to declare `getLocators` const?
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -74,10 +74,15 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
m_sniProxyHost(sniProxyHost),
m_sniProxyPort(sniProxyPort) {
for (auto&& locatorAddress : locatorAddresses) {
- m_locHostPort.emplace_back(locatorAddress);
+ m_Locators.emplace_back(locatorAddress);
}
}
+std::vector<ServerLocation> ThinClientLocatorHelper::getLocators() const {
Review comment:
This would be a great place for a comment block explaining why we're doing what we're doing, and why it's a bad idea to just return a reference to m_Locators. These sorts of things tend to be forgotten over time.
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -267,24 +263,20 @@ GfErrType ThinClientLocatorHelper::getEndpointForNewFwdConn(
LOGFINER(
"ThinClientLocatorHelper::getEndpointForNewFwdConn locatorsRetry = %d ",
locatorsRetry);
- for (unsigned attempts = 0;
- attempts <
- (m_locHostPort.size() == 1 ? locatorsRetry : m_locHostPort.size());
- attempts++) {
- ServerLocation serLoc;
- if (m_locHostPort.size() == 1) {
- serLoc = m_locHostPort[0];
- } else {
- serLoc = m_locHostPort[attempts];
- }
+
+ auto locators = getLocators();
+ auto locatorsSize = locators.size();
+ auto maxAttempts = locatorsSize == 1 ? locatorsRetry : locatorsSize;
+
+ for (auto attempts = 0ULL; attempts < maxAttempts; ++attempts) {
+ const auto& loc = locatorsSize == 1 ? locators[0] : locators[attempts];
Review comment:
Same as above
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #660:
URL: https://github.com/apache/geode-native/pull/660
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-725303579
Sorry to bother you again, it has been quite a long time since this had any feedback. Is there anything else you would like me to do in order to get this ready?
Thanks!
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r498899089
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -74,10 +74,15 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
m_sniProxyHost(sniProxyHost),
m_sniProxyPort(sniProxyPort) {
for (auto&& locatorAddress : locatorAddresses) {
- m_locHostPort.emplace_back(locatorAddress);
+ m_Locators.emplace_back(locatorAddress);
}
}
+std::vector<ServerLocation> ThinClientLocatorHelper::getLocators() const {
Review comment:
`const` is absolutely correct here and please keep adding more `const`. I wish we could all agree to make C++ objects and methods `const` by default and require keyword to make them mutable. Immutability for the win!
----------------------------------------------------------------
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] mreddington commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497817525
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -74,10 +74,15 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
m_sniProxyHost(sniProxyHost),
m_sniProxyPort(sniProxyPort) {
for (auto&& locatorAddress : locatorAddresses) {
Review comment:
Same here, use vector copy or iterator range initialization.
----------------------------------------------------------------
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] mreddington commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497827243
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -406,24 +396,24 @@ GfErrType ThinClientLocatorHelper::updateLocators(
auto response =
std::dynamic_pointer_cast<LocatorListResponse>(di.readObject());
- auto locators = response->getLocators();
- if (locators.size() > 0) {
+ auto newLocators = response->getLocators();
+ if (!newLocators.empty()) {
RandGen randGen;
- std::random_shuffle(locators.begin(), locators.end(), randGen);
+ std::random_shuffle(newLocators.begin(), newLocators.end(), randGen);
Review comment:
We would be better served random shuffling every time. You could do us a favor by moving this into your handy-dandy `getLocators` method.
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r498901186
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -42,38 +43,95 @@ class Connector;
class ThinClientLocatorHelper {
public:
- ThinClientLocatorHelper(const std::vector<std::string>& locatorAddresses,
+ ThinClientLocatorHelper(const ThinClientLocatorHelper&) = delete;
+ ThinClientLocatorHelper& operator=(const ThinClientLocatorHelper&) = delete;
+
+ ThinClientLocatorHelper(const std::vector<std::string>& locators,
const ThinClientPoolDM* poolDM);
- ThinClientLocatorHelper(const std::vector<std::string>& locatorAddresses,
+ ThinClientLocatorHelper(const std::vector<std::string>& locators,
const std::string& sniProxyHost, int sniProxyPort,
const ThinClientPoolDM* poolDM);
GfErrType getEndpointForNewFwdConn(
ServerLocation& outEndpoint, std::string& additionalLoc,
const std::set<ServerLocation>& exclEndPts,
const std::string& serverGrp = "",
- const TcrConnection* currentServer = nullptr);
+ const TcrConnection* currentServer = nullptr) const;
GfErrType getEndpointForNewCallBackConn(
ClientProxyMembershipID& memId, std::list<ServerLocation>& outEndpoint,
std::string& additionalLoc, int redundancy,
- const std::set<ServerLocation>& exclEndPts, const std::string& serverGrp);
+ const std::set<ServerLocation>& exclEndPts,
+ const std::string& serverGrp) const;
GfErrType getAllServers(
std::vector<std::shared_ptr<ServerLocation> >& servers,
- const std::string& serverGrp);
- int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ const std::string& serverGrp) const;
+ int32_t getCurLocatorsNum() const {
+ return static_cast<int32_t>(locators_.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
- Connector* createConnection(Connector*& conn, const char* hostname,
- int32_t port,
- std::chrono::microseconds waitSeconds,
- int32_t maxBuffSizePool = 0);
- std::mutex m_locatorLock;
- std::vector<ServerLocation> m_locHostPort;
+ /**
+ * Auxiliary types
+ */
+
+ class ConnectionWrapper {
+ private:
+ Connector* conn_;
+
+ public:
+ ConnectionWrapper(const ConnectionWrapper&) = delete;
+ ConnectionWrapper& operator=(const ConnectionWrapper&) = delete;
+
+ explicit ConnectionWrapper(Connector* conn) : conn_(conn) {}
+ ConnectionWrapper(ConnectionWrapper&& other)
+ : conn_(std::move(other.conn_)) {}
+
+ ~ConnectionWrapper();
+
+ Connector* operator->() { return conn_; }
+ };
+
+ /**
+ * Returns the number of connections retries per request
+ * @return Number of connection retries towards locators
+ */
+ int getConnRetries() const;
Review comment:
Can we have negative retries? Seems like a good place for `size_t` or some other `uintX_t`.
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -42,38 +43,95 @@ class Connector;
class ThinClientLocatorHelper {
public:
- ThinClientLocatorHelper(const std::vector<std::string>& locatorAddresses,
+ ThinClientLocatorHelper(const ThinClientLocatorHelper&) = delete;
+ ThinClientLocatorHelper& operator=(const ThinClientLocatorHelper&) = delete;
+
+ ThinClientLocatorHelper(const std::vector<std::string>& locators,
const ThinClientPoolDM* poolDM);
- ThinClientLocatorHelper(const std::vector<std::string>& locatorAddresses,
+ ThinClientLocatorHelper(const std::vector<std::string>& locators,
const std::string& sniProxyHost, int sniProxyPort,
const ThinClientPoolDM* poolDM);
GfErrType getEndpointForNewFwdConn(
ServerLocation& outEndpoint, std::string& additionalLoc,
const std::set<ServerLocation>& exclEndPts,
const std::string& serverGrp = "",
- const TcrConnection* currentServer = nullptr);
+ const TcrConnection* currentServer = nullptr) const;
GfErrType getEndpointForNewCallBackConn(
ClientProxyMembershipID& memId, std::list<ServerLocation>& outEndpoint,
std::string& additionalLoc, int redundancy,
- const std::set<ServerLocation>& exclEndPts, const std::string& serverGrp);
+ const std::set<ServerLocation>& exclEndPts,
+ const std::string& serverGrp) const;
GfErrType getAllServers(
std::vector<std::shared_ptr<ServerLocation> >& servers,
- const std::string& serverGrp);
- int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ const std::string& serverGrp) const;
+ int32_t getCurLocatorsNum() const {
Review comment:
We should just change the signature to return `size_t`, this `int32_t` is just old code hanging around.
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r498903370
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -42,38 +43,95 @@ class Connector;
class ThinClientLocatorHelper {
public:
- ThinClientLocatorHelper(const std::vector<std::string>& locatorAddresses,
+ ThinClientLocatorHelper(const ThinClientLocatorHelper&) = delete;
+ ThinClientLocatorHelper& operator=(const ThinClientLocatorHelper&) = delete;
+
+ ThinClientLocatorHelper(const std::vector<std::string>& locators,
const ThinClientPoolDM* poolDM);
- ThinClientLocatorHelper(const std::vector<std::string>& locatorAddresses,
+ ThinClientLocatorHelper(const std::vector<std::string>& locators,
const std::string& sniProxyHost, int sniProxyPort,
const ThinClientPoolDM* poolDM);
GfErrType getEndpointForNewFwdConn(
ServerLocation& outEndpoint, std::string& additionalLoc,
const std::set<ServerLocation>& exclEndPts,
const std::string& serverGrp = "",
- const TcrConnection* currentServer = nullptr);
+ const TcrConnection* currentServer = nullptr) const;
GfErrType getEndpointForNewCallBackConn(
ClientProxyMembershipID& memId, std::list<ServerLocation>& outEndpoint,
std::string& additionalLoc, int redundancy,
- const std::set<ServerLocation>& exclEndPts, const std::string& serverGrp);
+ const std::set<ServerLocation>& exclEndPts,
+ const std::string& serverGrp) const;
GfErrType getAllServers(
std::vector<std::shared_ptr<ServerLocation> >& servers,
- const std::string& serverGrp);
- int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ const std::string& serverGrp) const;
+ int32_t getCurLocatorsNum() const {
Review comment:
Sure. Makes total sense π
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497879294
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -406,24 +396,24 @@ GfErrType ThinClientLocatorHelper::updateLocators(
auto response =
std::dynamic_pointer_cast<LocatorListResponse>(di.readObject());
- auto locators = response->getLocators();
- if (locators.size() > 0) {
+ auto newLocators = response->getLocators();
+ if (!newLocators.empty()) {
RandGen randGen;
- std::random_shuffle(locators.begin(), locators.end(), randGen);
+ std::random_shuffle(newLocators.begin(), newLocators.end(), randGen);
Review comment:
Good point π
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-703800820
So sorry @pdxcodemonkey,
Will fix the issue and compile/run tests in all platforms, just to be sure. It might take a while.
----------------------------------------------------------------
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] mreddington commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497814893
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -74,10 +74,15 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
m_sniProxyHost(sniProxyHost),
m_sniProxyPort(sniProxyPort) {
for (auto&& locatorAddress : locatorAddresses) {
- m_locHostPort.emplace_back(locatorAddress);
+ m_Locators.emplace_back(locatorAddress);
}
}
+std::vector<ServerLocation> ThinClientLocatorHelper::getLocators() const {
Review comment:
No need to be const.
Put the random_shuffle here.
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497836960
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -62,7 +62,7 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
const ThinClientPoolDM* poolDM)
: m_poolDM(poolDM) {
for (auto&& locatorAddress : locatorAddresses) {
- m_locHostPort.emplace_back(locatorAddress);
+ m_Locators.emplace_back(locatorAddress);
Review comment:
Got it π
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497874123
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -267,24 +263,20 @@ GfErrType ThinClientLocatorHelper::getEndpointForNewFwdConn(
LOGFINER(
"ThinClientLocatorHelper::getEndpointForNewFwdConn locatorsRetry = %d ",
locatorsRetry);
- for (unsigned attempts = 0;
- attempts <
- (m_locHostPort.size() == 1 ? locatorsRetry : m_locHostPort.size());
- attempts++) {
- ServerLocation serLoc;
- if (m_locHostPort.size() == 1) {
- serLoc = m_locHostPort[0];
- } else {
- serLoc = m_locHostPort[attempts];
- }
+
+ auto locators = getLocators();
+ auto locatorsSize = locators.size();
+ auto maxAttempts = locatorsSize == 1 ? locatorsRetry : locatorsSize;
+
+ for (auto attempts = 0ULL; attempts < maxAttempts; ++attempts) {
+ const auto& loc = locatorsSize == 1 ? locators[0] : locators[attempts];
Review comment:
π
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-706753854
# [Codecov](https://codecov.io/gh/apache/geode-native/pull/660?src=pr&el=h1) Report
> Merging [#660](https://codecov.io/gh/apache/geode-native/pull/660?src=pr&el=desc) (ebb7cb8) into [develop](https://codecov.io/gh/apache/geode-native/commit/0d9a99d5e0632de62df17921950cf3f6640efb33?el=desc) (0d9a99d) will **increase** coverage by `0.00%`.
> The diff coverage is `94.65%`.
[](https://codecov.io/gh/apache/geode-native/pull/660?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## develop #660 +/- ##
=========================================
Coverage 74.04% 74.04%
=========================================
Files 644 644
Lines 51189 51086 -103
=========================================
- Hits 37903 37828 -75
+ Misses 13286 13258 -28
```
| [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/660?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [cppcache/src/ThinClientLocatorHelper.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRMb2NhdG9ySGVscGVyLmNwcA==) | `93.87% <94.44%> (+7.76%)` | :arrow_up: |
| [cppcache/src/ThinClientLocatorHelper.hpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRMb2NhdG9ySGVscGVyLmhwcA==) | `100.00% <100.00%> (ΓΈ)` | |
| [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `76.32% <100.00%> (-0.06%)` | :arrow_down: |
| [cppcache/src/ReadWriteLock.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1JlYWRXcml0ZUxvY2suY3Bw) | `62.50% <0.00%> (-18.75%)` | :arrow_down: |
| [cppcache/src/PdxTypeRegistry.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1BkeFR5cGVSZWdpc3RyeS5jcHA=) | `77.94% <0.00%> (-2.21%)` | :arrow_down: |
| [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `62.24% <0.00%> (-0.46%)` | :arrow_down: |
| [cppcache/src/ThinClientRedundancyManager.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWR1bmRhbmN5TWFuYWdlci5jcHA=) | `76.09% <0.00%> (-0.32%)` | :arrow_down: |
| [cppcache/src/TcrConnection.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckNvbm5lY3Rpb24uY3Bw) | `72.95% <0.00%> (+0.47%)` | :arrow_up: |
| [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `55.11% <0.00%> (+0.56%)` | :arrow_up: |
| ... and [2 more](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/660?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/660?src=pr&el=footer). Last update [0d9a99d...ebb7cb8](https://codecov.io/gh/apache/geode-native/pull/660?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] pivotal-jbarrett commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r498897942
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -60,20 +63,23 @@ class ThinClientLocatorHelper {
std::vector<std::shared_ptr<ServerLocation> >& servers,
const std::string& serverGrp);
int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ return static_cast<int32_t>(m_Locators.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
+ std::vector<ServerLocation> getLocators() const;
Connector* createConnection(Connector*& conn, const char* hostname,
int32_t port,
std::chrono::microseconds waitSeconds,
int32_t maxBuffSizePool = 0);
- std::mutex m_locatorLock;
- std::vector<ServerLocation> m_locHostPort;
+
+ /**
+ * Data members
+ */
+ mutable std::mutex m_locatorLock;
Review comment:
`mutable` on mutex, and the like, is not a code smell but actually an appropriate pattern. The `mutable` her indicates that this, and only this, value can mutate in an otherwise immutable object and that is correct because the mutex is not part of the state of the object but more metadata protecting this state.
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-702881347
I'm running tests on a Windows EC2 instance to confirm it's a real thing and not just a hiccup in our infrastructure.
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-702924369
I'm hitting the same test results on a Windows VM, so something major is wrong on that platform.
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-703280194
@gaussianrecurrence Still seeing a clangformat failure in ThinClientLocatorHelper.hpp, see Travis results above. Note that you need clangformat v6 or v7 to be compatible with the v6 we're using in Travis. When you have the right version installed on your dev machine, from a clean repo run `cmake --build . --target all-clangformat`, then check in any formatting changes the tool has made. Thanks!
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-702877590
Oh my... let me see if I can set up an environment in Windows and run all the tests. Sorry for the inconviniences :S
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r498903573
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -42,38 +43,95 @@ class Connector;
class ThinClientLocatorHelper {
public:
- ThinClientLocatorHelper(const std::vector<std::string>& locatorAddresses,
+ ThinClientLocatorHelper(const ThinClientLocatorHelper&) = delete;
+ ThinClientLocatorHelper& operator=(const ThinClientLocatorHelper&) = delete;
+
+ ThinClientLocatorHelper(const std::vector<std::string>& locators,
const ThinClientPoolDM* poolDM);
- ThinClientLocatorHelper(const std::vector<std::string>& locatorAddresses,
+ ThinClientLocatorHelper(const std::vector<std::string>& locators,
const std::string& sniProxyHost, int sniProxyPort,
const ThinClientPoolDM* poolDM);
GfErrType getEndpointForNewFwdConn(
ServerLocation& outEndpoint, std::string& additionalLoc,
const std::set<ServerLocation>& exclEndPts,
const std::string& serverGrp = "",
- const TcrConnection* currentServer = nullptr);
+ const TcrConnection* currentServer = nullptr) const;
GfErrType getEndpointForNewCallBackConn(
ClientProxyMembershipID& memId, std::list<ServerLocation>& outEndpoint,
std::string& additionalLoc, int redundancy,
- const std::set<ServerLocation>& exclEndPts, const std::string& serverGrp);
+ const std::set<ServerLocation>& exclEndPts,
+ const std::string& serverGrp) const;
GfErrType getAllServers(
std::vector<std::shared_ptr<ServerLocation> >& servers,
- const std::string& serverGrp);
- int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ const std::string& serverGrp) const;
+ int32_t getCurLocatorsNum() const {
+ return static_cast<int32_t>(locators_.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
- Connector* createConnection(Connector*& conn, const char* hostname,
- int32_t port,
- std::chrono::microseconds waitSeconds,
- int32_t maxBuffSizePool = 0);
- std::mutex m_locatorLock;
- std::vector<ServerLocation> m_locHostPort;
+ /**
+ * Auxiliary types
+ */
+
+ class ConnectionWrapper {
+ private:
+ Connector* conn_;
+
+ public:
+ ConnectionWrapper(const ConnectionWrapper&) = delete;
+ ConnectionWrapper& operator=(const ConnectionWrapper&) = delete;
+
+ explicit ConnectionWrapper(Connector* conn) : conn_(conn) {}
+ ConnectionWrapper(ConnectionWrapper&& other)
+ : conn_(std::move(other.conn_)) {}
+
+ ~ConnectionWrapper();
+
+ Connector* operator->() { return conn_; }
+ };
+
+ /**
+ * Returns the number of connections retries per request
+ * @return Number of connection retries towards locators
+ */
+ int getConnRetries() const;
Review comment:
Indeed π
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-702792203
> I'll think about your idea today and get back to you. My first instinct is that it's a much larger change than even the initial one, however, so it may be beyond the scope of what I'm comfortable with, absent a _lot_ more testing etc.
>
> In the meantime, your last commit broke our Windows build, maybe due to some platform difference in boost(?). Here's the error output:
>
> ```
> ThinClientPoolStickyDM.cpp
> C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(73): error C2039: 'shared_lock': is not a member of 'boost' [C:\build\cppcache\static\apache-geode-static.vcxproj]
> C:\build\dependencies\boost\RelWithDebInfo\include\boost/thread/shared_mutex.hpp(36): note: see declaration of 'boost'
> C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(73): error C2065: 'shared_lock': undeclared identifier [C:\build\cppcache\static\apache-geode-static.vcxproj]
> C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(73): error C2226: syntax error: unexpected type 'boost::shared_mutex' [C:\build\cppcache\static\apache-geode-static.vcxproj]
> C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(73): error C2143: syntax error: missing ';' before '{' [C:\build\cppcache\static\apache-geode-static.vcxproj]
> C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(73): error C2143: syntax error: missing ';' before '}' [C:\build\cppcache\static\apache-geode-static.vcxproj]
> C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(303): error C2039: 'unique_lock': is not a member of 'boost' [C:\build\cppcache\static\apache-geode-static.vcxproj]
> C:\build\dependencies\boost\RelWithDebInfo\include\boost/thread/shared_mutex.hpp(36): note: see declaration of 'boost'
> C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(303): error C2065: 'unique_lock': undeclared identifier [C:\build\cppcache\static\apache-geode-static.vcxproj]
> C:\nativeclient\cppcache\src\ThinClientLocatorHelper.cpp(303): error C2226: syntax error: unexpected type 'boost::shared_mutex' [C:\build\cppcache\static\apache-geode-static.vcxproj]
> ```
>
> I'll try a build on a Windows instance here, and let you know if I find out what's up before you. Thanks!
Regarding about the size of the PR, totally agree. Indeed usually I tend to do many more but much smaller commits. But I don't know what's your WOW here. I guess we could always split this PR right?
Regarding the Windows error, sorry, my mistake, Thing is Boost.Thread includes different headers depending on the platform. Pthread implementation implicitly includes boost/thread/lock_types.hpp, but the windows implementation does not. I've already pushed a commit solving it.
Btw. Do you have a publicly available CI cluster or somwhere were test can be executed for every supported platform?
Or are we supposed to execute them all manually?
Thanks for all :)
----------------------------------------------------------------
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] gaussianrecurrence edited a comment on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence edited a comment on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-702736992
Hi again,
Regarding the test proposal, I've got an idea I would like to share with you in order to get your opinion on the matter.
The purpose of this PR is avoid that whenever a thread is connecting to a locator or performing a request, other threads accesing the pool locator **don't get locked**.
Therefore what I thought is to:
- Introduce a new abstract class **ConnectorFactory**, which basically creates a new **Connector**.
- Create a **ConnectorFactory** implementation called **TcpConnFactory** which basically creates **TcpConn**/**TcpSslConn**. This itself would also help reduced duplicated code, as connections creation is duplicated both in **TcrConnection** and **ThinClientLocatorHelper**.
- PoolFactory would be the one responsible for setting the instance of ConnectorFactory into the created pool, adding therefore a method to set ConnectorFactory instance in there.
- ConnectorFactory should be then accesible throught the Pool.
- Create a Google Mock for ConnectorFactory.
- Create a Google Mock for Connector.
**PST.** While seeking a solution for the test proposal I think I've noticed something: "Is it possible that many of the factory class existing in NC, should be instead builders?"
Ok, so having set all the above scaffolding, then the idea would be to create a ThinClientLocatorHelper TS which tries to perform requests by multiple threads, and having the ability to control whats returned by the connections and how much time is spent on a connection operation, you can therefore check that if one thread is waiting for an operation to complete, the other thread can inmmediatly return.
An pseudo-code example would be the following:
**testGetAllServerWhileUpdating**
`
createCache();
createPoolFactoryWithCustomConnectorFactory();
updateT = createUpdateThread(connector_sleep_time=10s);
getAllServersT = createGetAllServersThread(connector_sleep_time=0s);
ASSERT_EQ(getAllServersT.wait_for(1s), std::future_state::ready);
`
---
So want do you think about it?
What I don't know is where would I put the test, for that I might need a hand from your side :)
BR
/Mario
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-705153528
Re: BasicIPV6Test, you're right this one won't pass unless your build is compiled WITH_IPV6 _and_ your network is configured to route IPv6. We've never had a test bed that worked for this one, so in our CI we use the `ctest -E` switch to avoid running the test.
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-703330897
> @gaussianrecurrence Still seeing a clangformat failure in ThinClientLocatorHelper.hpp, see Travis results above. Note that you need clangformat v6 or v7 to be compatible with the v6 we're using in Travis. When you have the right version installed on your dev machine, from a clean repo run `cmake --build . --target all-clangformat`, then check in any formatting changes the tool has made. Thanks!
Yes, sorry I made the change quickly and forgot that :S
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497865452
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -60,20 +63,23 @@ class ThinClientLocatorHelper {
std::vector<std::shared_ptr<ServerLocation> >& servers,
const std::string& serverGrp);
int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ return static_cast<int32_t>(m_Locators.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
+ std::vector<ServerLocation> getLocators() const;
Connector* createConnection(Connector*& conn, const char* hostname,
int32_t port,
std::chrono::microseconds waitSeconds,
int32_t maxBuffSizePool = 0);
- std::mutex m_locatorLock;
- std::vector<ServerLocation> m_locHostPort;
+
+ /**
+ * Data members
+ */
+ mutable std::mutex m_locatorLock;
Review comment:
> The `mutable` keyword is usually a "code smell" - is this just needed in order to declare `getLocators` const?
As stated above I don't agree on getLocators no being a const function member.
With this premise, as the object state is not changed per-se, and you are using a mutex as a tool to guarantee mutual exclusion, it would make sense for the mutex to be mutable, so lock/unlock operations are allows in a const-qualified scope.
Regarding this very case, I've seen Herb Sutter has a "M&M rule" which basically summarizes to "mutexes are to be mutable", you can see that here: https://herbsutter.com/2013/05/24/gotw-6a-const-correctness-part-1-3/
Also, I am awaiting on the response for other C++ comitee members opinion on this matter.
I get your point about the code-smell, but in this case marking it as such, it doesn't feel right
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-701732991
Regarding general comment about the mssing test ensuring this PR is not breaking anything, let me give it a thought and will come back with a proposal :)
----------------------------------------------------------------
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] mreddington commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497814345
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -60,20 +63,23 @@ class ThinClientLocatorHelper {
std::vector<std::shared_ptr<ServerLocation> >& servers,
const std::string& serverGrp);
int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ return static_cast<int32_t>(m_Locators.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
+ std::vector<ServerLocation> getLocators() const;
Review comment:
No need to make this method const as it's not called from any const method.
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r498430003
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -60,20 +63,23 @@ class ThinClientLocatorHelper {
std::vector<std::shared_ptr<ServerLocation> >& servers,
const std::string& serverGrp);
int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ return static_cast<int32_t>(m_Locators.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
+ std::vector<ServerLocation> getLocators() const;
Connector* createConnection(Connector*& conn, const char* hostname,
int32_t port,
std::chrono::microseconds waitSeconds,
int32_t maxBuffSizePool = 0);
- std::mutex m_locatorLock;
- std::vector<ServerLocation> m_locHostPort;
+
+ /**
+ * Data members
+ */
+ mutable std::mutex m_locatorLock;
Review comment:
shared_mutex seems like a good call, from my reading of it. Or go with what you have, if you prefer a minimal change.
Just to be clear with respect to getLocators, I wasn't suggesting we remove the const qualifier from the method. I was just curious if that is why the mutex needed to be mutable. Your answer is perfectly fine and quite thorough, so leave the mutex variable as mutable, and I'll resolve this conversation.
----------------------------------------------------------------
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 #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-706753854
# [Codecov](https://codecov.io/gh/apache/geode-native/pull/660?src=pr&el=h1) Report
> Merging [#660](https://codecov.io/gh/apache/geode-native/pull/660?src=pr&el=desc) into [develop](https://codecov.io/gh/apache/geode-native/commit/5f06ae2044004397492249fcf13bcd90e75b955b?el=desc) will **decrease** coverage by `0.07%`.
> The diff coverage is `93.12%`.
[](https://codecov.io/gh/apache/geode-native/pull/660?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## develop #660 +/- ##
===========================================
- Coverage 74.03% 73.95% -0.08%
===========================================
Files 644 644
Lines 51133 51030 -103
===========================================
- Hits 37854 37740 -114
- Misses 13279 13290 +11
```
| [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/660?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [cppcache/src/ThinClientLocatorHelper.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRMb2NhdG9ySGVscGVyLmNwcA==) | `91.15% <92.85%> (+5.04%)` | :arrow_up: |
| [cppcache/src/ThinClientLocatorHelper.hpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRMb2NhdG9ySGVscGVyLmhwcA==) | `100.00% <100.00%> (ΓΈ)` | |
| [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `75.46% <100.00%> (-0.63%)` | :arrow_down: |
| [...tion-test/testThinClientRemoteQueryFailoverPdx.cpp](https://codecov.io/gh/apache/geode-native/pull/660/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/660/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/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `54.55% <0.00%> (-1.83%)` | :arrow_down: |
| [cppcache/src/RemoteQuery.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1JlbW90ZVF1ZXJ5LmNwcA==) | `75.53% <0.00%> (-1.07%)` | :arrow_down: |
| [cppcache/src/ThinClientRedundancyManager.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWR1bmRhbmN5TWFuYWdlci5jcHA=) | `75.31% <0.00%> (-0.47%)` | :arrow_down: |
| [cppcache/src/TcrConnection.cpp](https://codecov.io/gh/apache/geode-native/pull/660/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckNvbm5lY3Rpb24uY3Bw) | `72.79% <0.00%> (-0.16%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/660?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/660?src=pr&el=footer). Last update [5f06ae2...1f3142d](https://codecov.io/gh/apache/geode-native/pull/660?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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-702736992
Hi again,
Regarding the test proposal, I've got an idea I would like to share with you in order to get your opinion on the matter.
The purpose of this PR is avoid that whenever a thread is connecting to a locator or performing a request, other threads accesing the pool locator **don't get locked**.
Therefore what I thought is to:
- Introduce a new abstract class **ConnectorFactory**, which basically creates a new **Connector**.
- Create a **ConnectorFactory** implementation called **TcpConnFactory** which basically creates **TcpConn**/**TcpSslConn**. This itself would also help reduced duplicated code, as connections creation is duplicated both in **TcrConnection** and **ThinClientLocatorHelper**.
- PoolFactory would be the one responsible for setting the instance of ConnectorFactory into the created pool, adding therefore a method to set ConnectorFactory instance in there.
- ConnectorFactory should be then accesible throught the Pool.
- Create a Google Mock for ConnectorFactory.
- Create a Google Mock for Connector.
**PST.** While seeking a solution for the test proposal I think I've noticed something: "Is it possible that many of the factory class existing in NC, should be instead builders?"
Ok, so having set all the above scaffolding, then the idea would be to create a ThinClientLocatorHelper TS which tries to perform requests by multiple threads, and having the ability to control whats returned by the connections and how much time is spent on a connection operation, you can therefore check that if one thread is waiting for an operation to complete, the other thread can inmmediatly return.
An pseudo-code example would be the following:
**testGetAllServerWhileUpdating**
`
createCache();
createPoolFactoryWithCustomConnectorFactory();
updateT = createUpdateThread(connector_sleep_time=10s);
getAllServersT = createGetAllServersThread(connector_sleep_time=0s);
ASSERT_EQ(getAllServersT.wait_for(1s), std::future_state::ready);
`
---
So want do you think about it?
What I don't know is where would I put the test, for that I might need a hand from your side :)
BR
/Mario
----------------------------------------------------------------
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] gaussianrecurrence commented on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-706738731
- Branch rebased.
- Clang-format passed
- Compilation ran OK both in Linux and Windows.
- All tests passed both in Linux and Windows.
Please let me know your feeling about the pending topic (the tests). If you think I might need to come up with an intermediate test, or maybe I should make a prior PR to setup all the necessary scaffolding in order to make connection tests in the future.
Thanks π
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497837894
##########
File path: cppcache/src/ThinClientLocatorHelper.cpp
##########
@@ -74,10 +74,15 @@ ThinClientLocatorHelper::ThinClientLocatorHelper(
m_sniProxyHost(sniProxyHost),
m_sniProxyPort(sniProxyPort) {
for (auto&& locatorAddress : locatorAddresses) {
- m_locHostPort.emplace_back(locatorAddress);
+ m_Locators.emplace_back(locatorAddress);
}
}
+std::vector<ServerLocation> ThinClientLocatorHelper::getLocators() const {
Review comment:
Good point π
----------------------------------------------------------------
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] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497876036
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -60,20 +63,23 @@ class ThinClientLocatorHelper {
std::vector<std::shared_ptr<ServerLocation> >& servers,
const std::string& serverGrp);
int32_t getCurLocatorsNum() {
- return static_cast<int32_t>(m_locHostPort.size());
+ return static_cast<int32_t>(m_Locators.size());
}
GfErrType updateLocators(const std::string& serverGrp = "");
private:
+ std::vector<ServerLocation> getLocators() const;
Review comment:
Please take a look at [my comment](https://github.com/apache/geode-native/pull/660#discussion_r497868084) to @pdxcodemonkey regarding this topic :)
Just to summarize: "If it were for me, everything but updateLocators would be const-qualified"
----------------------------------------------------------------
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