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/12/08 23:11:44 UTC
[GitHub] [geode-native] mreddington opened a new pull request #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
mreddington opened a new pull request #706:
URL: https://github.com/apache/geode-native/pull/706
----------------------------------------------------------------
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 #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #706:
URL: https://github.com/apache/geode-native/pull/706#discussion_r539421630
##########
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:
Done.
----------------------------------------------------------------
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 #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #706:
URL: https://github.com/apache/geode-native/pull/706#issuecomment-741862633
Per the larger team, Java side is not using 0 for these, only for certain server-initiated messages. Java client sets NOTX for PING and CLOSE_CONNECTION, native client should as well.
----------------------------------------------------------------
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 #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #706:
URL: https://github.com/apache/geode-native/pull/706#issuecomment-741209609
@dihardman @karensmolermiller @davebarnes97
----------------------------------------------------------------
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 #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #706:
URL: https://github.com/apache/geode-native/pull/706
----------------------------------------------------------------
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 pull request #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #706:
URL: https://github.com/apache/geode-native/pull/706#issuecomment-741277312
> The fix should be made in the Geode server sources at make BaseCommand.shouldMasqueradeForTx consistent with all other checks against `NOTX`. In all other checks explicit equality is checked, not for greater or less than.
Pointed out offline to me, this won't fix the original issue either since `0 != NOTX` is `true` too. 🤦
I really think we should understand why `0` was used here and on both the C++ and Java versions of these messages.
----------------------------------------------------------------
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 #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
Posted by 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
[GitHub] [geode-native] codecov-io commented on pull request #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #706:
URL: https://github.com/apache/geode-native/pull/706#issuecomment-741369633
# [Codecov](https://codecov.io/gh/apache/geode-native/pull/706?src=pr&el=h1) Report
> Merging [#706](https://codecov.io/gh/apache/geode-native/pull/706?src=pr&el=desc) (c43fc2c) into [develop](https://codecov.io/gh/apache/geode-native/commit/03f6031cbf9aa2dde46216cfebbaa8a4b46d4182?el=desc) (03f6031) will **decrease** coverage by `0.03%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/geode-native/pull/706?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## develop #706 +/- ##
===========================================
- Coverage 74.13% 74.09% -0.04%
===========================================
Files 644 644
Lines 50831 50830 -1
===========================================
- Hits 37683 37664 -19
- Misses 13148 13166 +18
```
| [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/706?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [cppcache/src/TcrMessage.cpp](https://codecov.io/gh/apache/geode-native/pull/706/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1Rjck1lc3NhZ2UuY3Bw) | `85.61% <100.00%> (-0.01%)` | :arrow_down: |
| [cppcache/src/ReadWriteLock.cpp](https://codecov.io/gh/apache/geode-native/pull/706/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1JlYWRXcml0ZUxvY2suY3Bw) | `81.25% <0.00%> (-18.75%)` | :arrow_down: |
| [...tion-test/testThinClientRemoteQueryFailoverPdx.cpp](https://codecov.io/gh/apache/geode-native/pull/706/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFJlbW90ZVF1ZXJ5RmFpbG92ZXJQZHguY3Bw) | `85.48% <0.00%> (-4.04%)` | :arrow_down: |
| [cppcache/src/ThinClientPoolDM.hpp](https://codecov.io/gh/apache/geode-native/pull/706/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uaHBw) | `90.32% <0.00%> (-1.62%)` | :arrow_down: |
| [cppcache/src/RemoteQuery.cpp](https://codecov.io/gh/apache/geode-native/pull/706/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1JlbW90ZVF1ZXJ5LmNwcA==) | `75.53% <0.00%> (-1.07%)` | :arrow_down: |
| [cppcache/src/TcpConn.cpp](https://codecov.io/gh/apache/geode-native/pull/706/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjcENvbm4uY3Bw) | `83.58% <0.00%> (-0.75%)` | :arrow_down: |
| [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/706/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `77.30% <0.00%> (-0.72%)` | :arrow_down: |
| [cppcache/src/ClientMetadata.cpp](https://codecov.io/gh/apache/geode-native/pull/706/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhLmNwcA==) | `65.16% <0.00%> (-0.57%)` | :arrow_down: |
| [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/706/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `62.70% <0.00%> (+0.22%)` | :arrow_up: |
| [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/706/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `55.53% <0.00%> (+0.42%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/706?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/706?src=pr&el=footer). Last update [03f6031...c43fc2c](https://codecov.io/gh/apache/geode-native/pull/706?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 #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #706:
URL: https://github.com/apache/geode-native/pull/706#discussion_r539515389
##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -2090,17 +2090,16 @@ TcrMessageReply::TcrMessageReply(bool decodeAll,
TcrMessagePing::TcrMessagePing(DataOutput* dataOutput, bool decodeAll) {
m_msgType = TcrMessage::PING;
+ // -1 is NOTX constant server-side, PING should *never* be part of a
+ // transaction
+ m_txId = -1;
Review comment:
Isn't this the default? Look at `TcrMessage` default constructor.
##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -2090,17 +2090,16 @@ TcrMessageReply::TcrMessageReply(bool decodeAll,
TcrMessagePing::TcrMessagePing(DataOutput* dataOutput, bool decodeAll) {
m_msgType = TcrMessage::PING;
+ // -1 is NOTX constant server-side, PING should *never* be part of a
+ // transaction
+ m_txId = -1;
m_decodeAll = decodeAll;
m_request.reset(dataOutput);
m_request->writeInt(m_msgType);
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:
Look at `TcrMessage::writeHeader` and see that almost all the messages are using this method to do some of the message bits in here. Perhaps it makes sense to normalize this message to match.
##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -2109,13 +2108,16 @@ TcrMessagePing::TcrMessagePing(DataOutput* dataOutput, bool decodeAll) {
TcrMessageCloseConnection::TcrMessageCloseConnection(DataOutput* dataOutput,
bool decodeAll) {
m_msgType = TcrMessage::CLOSE_CONNECTION;
+ // -1 is NOTX constant server-side, CLOSE_CONNECTION should *never* be part
+ // of a transaction
+ m_txId = -1;
m_decodeAll = decodeAll;
m_request.reset(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:
See comment about this in ping message.
----------------------------------------------------------------
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 #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #706:
URL: https://github.com/apache/geode-native/pull/706#discussion_r539421503
##########
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:
Switched to a simple unit test, framework changes are gone.
----------------------------------------------------------------
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 pull request #706: GEODE-8773: Test that PING and CLOSE_CONNECTION are not implicitly transactional.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #706:
URL: https://github.com/apache/geode-native/pull/706#issuecomment-741940925
Should we also have a `NOTX` constant like the Java client?
----------------------------------------------------------------
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