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 2021/04/28 00:07:54 UTC
[GitHub] [geode-native] pdxcodemonkey opened a new pull request #793: GEODE-9200: Clean up DisMess
pdxcodemonkey opened a new pull request #793:
URL: https://github.com/apache/geode-native/pull/793
- Remove static endpoint disconnected message
- use dynamically allocated message for this, a la other messages
- no longer leak an instance at shutdown, no more special case code for
this message when deleting messages.
- Remove some really unfortunate abbreviations in a couple of function and variable names.
--
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 #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #793:
URL: https://github.com/apache/geode-native/pull/793#discussion_r622488201
##########
File path: cppcache/src/ThinClientPoolHADM.cpp
##########
@@ -292,11 +293,11 @@ void ThinClientPoolHADM::removeCallbackConnection(TcrEndpoint* ep) {
m_redundancyManager->removeCallbackConnection(ep);
}
-void ThinClientPoolHADM::sendNotConMesToAllregions() {
+void ThinClientPoolHADM::sendNotConnectedMessageToAllRegions() {
std::lock_guard<decltype(m_regionsLock)> guard(m_regionsLock);
for (std::list<ThinClientRegion*>::iterator it = m_regions.begin();
Review comment:
Can this be changed to a for range loop?
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -2967,7 +2969,8 @@ void ThinClientRegion::executeFunction(
} else if (err == GF_NOTCON) {
attempt++;
LOGDEBUG(
- "ThinClientRegion::executeFunction with GF_NOTCON retry attempt = "
+ "ThinClientRegion::executeFunction with GF_NOTCON retry attempt "
Review comment:
More odd formatting changes.
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -2772,7 +2774,7 @@ void ThinClientRegion::receiveNotification(TcrMessage* msg) {
}
lock.unlock();
- if (TcrMessage::getAllEPDisMess() != msg) _GEODE_SAFE_DELETE(msg);
+ _GEODE_SAFE_DELETE(msg);
Review comment:
Any easy opportunity to make `msg` a smart pointer?
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -2723,12 +2727,12 @@ GfErrType ThinClientRegion::handleServerException(
"TransactionDataNodeHasDepartedException") !=
std::string::npos) {
error = GF_TRANSACTION_DATA_NODE_HAS_DEPARTED_EXCEPTION;
- } else if (exceptionMsg.find(
- "org.apache.geode.cache.TransactionDataRebalancedException") !=
+ } else if (exceptionMsg.find("org.apache.geode.cache."
+ "TransactionDataRebalancedException") !=
Review comment:
This feels like an unintentional or at least less readable format change.
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -3556,9 +3564,9 @@ void ChunkedFunctionExecutionResponse::handleChunk(
return;
}
- auto startLen = static_cast<size_t>(
- input.getBytesRead() -
- 1); // from here need to look value part + memberid AND -1 for array type
+ auto startLen = static_cast<size_t>(input.getBytesRead() -
+ 1); // from here need to look value part
Review comment:
Move the comment above so the formatting is more readable.
--
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 #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #793:
URL: https://github.com/apache/geode-native/pull/793
--
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 #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #793:
URL: https://github.com/apache/geode-native/pull/793#discussion_r622562195
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -2967,7 +2969,8 @@ void ThinClientRegion::executeFunction(
} else if (err == GF_NOTCON) {
attempt++;
LOGDEBUG(
- "ThinClientRegion::executeFunction with GF_NOTCON retry attempt = "
+ "ThinClientRegion::executeFunction with GF_NOTCON retry attempt "
Review comment:
That is even an odd one from clang-format on windows since it is splitting a sting 2 characters shorter. 🤷
--
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 #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #793:
URL: https://github.com/apache/geode-native/pull/793#discussion_r622595152
##########
File path: cppcache/src/CacheImpl.cpp
##########
@@ -706,16 +706,16 @@ void CacheImpl::processMarker() {
if (!kv.second->isDestroyed()) {
if (const auto tcrHARegion =
std::dynamic_pointer_cast<ThinClientHARegion>(kv.second)) {
- auto regionMsg = new TcrMessageClientMarker(
- new DataOutput(createDataOutput()), true);
- tcrHARegion->receiveNotification(regionMsg);
+ tcrHARegion->receiveNotification(
+ std::unique_ptr<TcrMessage>(new TcrMessageClientMarker(
+ new DataOutput(createDataOutput()), true)));
Review comment:
Can we refactor away this constant `true` 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] pdxcodemonkey commented on a change in pull request #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #793:
URL: https://github.com/apache/geode-native/pull/793#discussion_r622518629
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -2967,7 +2969,8 @@ void ThinClientRegion::executeFunction(
} else if (err == GF_NOTCON) {
attempt++;
LOGDEBUG(
- "ThinClientRegion::executeFunction with GF_NOTCON retry attempt = "
+ "ThinClientRegion::executeFunction with GF_NOTCON retry attempt "
Review comment:
Really wish I knew why this was happening. It's as if `clang-format` v11 on Windows is not exactly the same as `clang-format` v11 on other platforms. It's frustrating.
--
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 #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #793:
URL: https://github.com/apache/geode-native/pull/793#discussion_r622585525
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -2772,7 +2774,7 @@ void ThinClientRegion::receiveNotification(TcrMessage* msg) {
}
lock.unlock();
- if (TcrMessage::getAllEPDisMess() != msg) _GEODE_SAFE_DELETE(msg);
+ _GEODE_SAFE_DELETE(msg);
Review comment:
This one was a little larger than I anticipated, but I think it's a really nice refactor.
--
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 #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #793:
URL: https://github.com/apache/geode-native/pull/793#discussion_r622519109
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -2672,25 +2672,29 @@ GfErrType ThinClientRegion::clientNotificationHandler(TcrMessage& msg) {
LocalRegion::tombstoneOperationNoThrow(msg.getTombstoneVersions(),
msg.getTombstoneKeys());
break;
- default: {
- if (TcrMessage::getAllEPDisMess() == &msg) {
+ default:
+ try {
+ auto& marker =
+ dynamic_cast<TcrMessageAllEndpointsDisconnectedMarker&>(msg);
setProcessedMarker(false);
+ LOGDEBUG(
+ "ThinClientRegion::clientNotificationHandler: rec'd endpoints "
+ "disconnected message");
LocalRegion::invokeAfterAllEndPointDisconnected();
- } else {
+ } catch (std::bad_cast) {
Review comment:
Yeah, LGTM griped at me about this 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] pdxcodemonkey commented on a change in pull request #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #793:
URL: https://github.com/apache/geode-native/pull/793#discussion_r622304986
##########
File path: cppcache/src/ThinClientPoolHADM.cpp
##########
@@ -292,11 +293,11 @@ void ThinClientPoolHADM::removeCallbackConnection(TcrEndpoint* ep) {
m_redundancyManager->removeCallbackConnection(ep);
}
-void ThinClientPoolHADM::sendNotConMesToAllregions() {
+void ThinClientPoolHADM::sendNotConnectedMessageToAllregions() {
Review comment:
Fixed. Nice catch!
--
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 #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #793:
URL: https://github.com/apache/geode-native/pull/793#discussion_r622502904
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -2672,25 +2672,29 @@ GfErrType ThinClientRegion::clientNotificationHandler(TcrMessage& msg) {
LocalRegion::tombstoneOperationNoThrow(msg.getTombstoneVersions(),
msg.getTombstoneKeys());
break;
- default: {
- if (TcrMessage::getAllEPDisMess() == &msg) {
+ default:
+ try {
+ auto& marker =
+ dynamic_cast<TcrMessageAllEndpointsDisconnectedMarker&>(msg);
setProcessedMarker(false);
+ LOGDEBUG(
+ "ThinClientRegion::clientNotificationHandler: rec'd endpoints "
+ "disconnected message");
LocalRegion::invokeAfterAllEndPointDisconnected();
- } else {
+ } catch (std::bad_cast) {
Review comment:
```c++
} catch (std::bad_cast&) {
```
--
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] mmartell commented on a change in pull request #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #793:
URL: https://github.com/apache/geode-native/pull/793#discussion_r622255695
##########
File path: cppcache/src/ThinClientPoolHADM.cpp
##########
@@ -292,11 +293,11 @@ void ThinClientPoolHADM::removeCallbackConnection(TcrEndpoint* ep) {
m_redundancyManager->removeCallbackConnection(ep);
}
-void ThinClientPoolHADM::sendNotConMesToAllregions() {
+void ThinClientPoolHADM::sendNotConnectedMessageToAllregions() {
Review comment:
ToAllRegions not ToAllregions
--
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 #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #793:
URL: https://github.com/apache/geode-native/pull/793#discussion_r622551469
##########
File path: cppcache/src/ThinClientPoolHADM.cpp
##########
@@ -292,11 +293,11 @@ void ThinClientPoolHADM::removeCallbackConnection(TcrEndpoint* ep) {
m_redundancyManager->removeCallbackConnection(ep);
}
-void ThinClientPoolHADM::sendNotConMesToAllregions() {
+void ThinClientPoolHADM::sendNotConnectedMessageToAllRegions() {
std::lock_guard<decltype(m_regionsLock)> guard(m_regionsLock);
for (std::list<ThinClientRegion*>::iterator it = m_regions.begin();
Review comment:
Yep, fixed.
--
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 #793: GEODE-9200: Clean up DisMess
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #793:
URL: https://github.com/apache/geode-native/pull/793#issuecomment-828743251
This pull request **introduces 1 alert** when merging 42f79a127a85d72440582653acf1b68039451a79 into 0f40b8d8b5d530c5f650593526c01c0185d5efba - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-20cf132a5d1054aef5f477c77fcd57c896561cc6)
**new alerts:**
* 1 for Catching by value
--
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