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 20:53:36 UTC
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator
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