[
https://issues.apache.org/jira/browse/GEODE-8773?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17246187#comment-17246187
]
ASF GitHub Bot commented on GEODE-8773:
---------------------------------------
pivotal-jbarrett commented on a change in pull request #706:
URL: https://github.com/apache/geode-native/pull/706#discussion_r538883695
##########
File path: cppcache/integration/framework/Cluster.cpp
##########
@@ -110,6 +109,12 @@ void Locator::start() {
.withPreferIPv6(cluster_.getUseIPv6())
.withJmxManagerStart(true);
+ if(cluster_.getLogLevel().empty()) {
Review comment:
Why pull this out of the `.withLogLevel` model? Why not have cluster
default non-empty value and then always pass cluster value through to
`gfsh.withLogLevel()`?
##########
File path: cppcache/integration/test/TransactionsTest.cpp
##########
@@ -36,6 +40,30 @@ using apache::geode::client::Pool;
using apache::geode::client::Region;
using apache::geode::client::RegionShortcut;
+std::string getServerLogName() {
Review comment:
Using logs to validate tests is really dangerous. Log messages change
and aren't part of API. Is there a more concrete way to test this? I would
think a unit tests that asserts these two methods set the correct 'no tx' value
of `-1` should be sufficient.
##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -2096,14 +2096,9 @@ TcrMessagePing::TcrMessagePing(DataOutput* dataOutput,
bool decodeAll) {
m_request->writeInt(static_cast<int32_t>(
0)); // 17 is fixed message len ... PING only has a header.
m_request->writeInt(static_cast<int32_t>(0)); // Number of parts.
- // int32_t txId = TcrMessage::m_transactionId++;
- // Setting the txId to 0 for all ping message as it is not being used on the
- // SERVER side or the
- // client side.
- m_request->writeInt(static_cast<int32_t>(0));
+ m_request->writeInt(m_txId);
Review comment:
A ping is not a transactional item. It should send `-1`.
##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -2114,8 +2109,7 @@
TcrMessageCloseConnection::TcrMessageCloseConnection(DataOutput* dataOutput,
m_request->writeInt(m_msgType);
m_request->writeInt(static_cast<int32_t>(6));
m_request->writeInt(static_cast<int32_t>(1)); // Number of parts.
- // int32_t txId = TcrMessage::m_transactionId++;
- m_request->writeInt(static_cast<int32_t>(0));
+ m_request->writeInt(m_txId);
Review comment:
Not transactional, should send `-1`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Unnecessary transaction overhead being created on server
> --------------------------------------------------------
>
> Key: GEODE-8773
> URL: https://issues.apache.org/jira/browse/GEODE-8773
> Project: Geode
> Issue Type: Bug
> Components: native client
> Reporter: Blake Bender
> Assignee: Blake Bender
> Priority: Major
> Labels: pull-request-available
>
> This is a _very_ old bug, that was just discovered. In the Geode protocol,
> no matter which message is being handled, a Geode server will examine the
> “transaction Id” value in the message header, and if it is not the symbolic
> constant “NOTX” (aka -1), will assume the message is part of a transaction
> with that Id, and _set up resources for a transaction if one with that Id
> doesn’t already exist_. Said resource can include a high priority thread. In
> conjunction with this, we have the fact that the native client goes out of
> its way to always set the transaction id to 0 for PING and CLOSE_CONNECTION
> messages. The last ingredient for reproducing this problem is to have a whole
> bunch (like on the order of several hundred or several thousand) client
> applications start up and exit in a short period of time. Each will send at
> least one CLOSE_CONNECTION message, all with transaction Id 0, thus each
> client instance will leave one dangling transaction on the server. Eventually
> these will cause the server to spin up a fatal number of high-priority
> threads.
> The fix in the native client is trivial, just stop writing 0 instead of the
> transaction ID in the PING and CLOSE_CONNECTION messages.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
