[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org