Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GEODE-8480: Add txmanager check in tx example #648

Merged
merged 3 commits into from Oct 6, 2020

Conversation

alb3rtobr
Copy link
Contributor

No description provided.

@pdxcodemonkey
Copy link
Contributor

Did you find an issue that was fixed by this? What is the repro case? Can we add a simple test that shows the problem? The changes look fine, but it's difficult to judge correctness or whether there's a product issue since it doesn't modify any product code.

@alb3rtobr
Copy link
Contributor Author

I was running a test consisting on killing a Geode server while a C++ client with several threads was executing transactions. I observed that most of the times, the client died after killing the server with following error in logs:

[error 2020/09/01 10:31:05.977905 CEST alb3rtobr-XPS:7395 139697166743296] Unexpected exception during completing transaction : illegal State
terminate called after throwing an instance of 'apache::geode::client::Exception'
terminate called recursively
  what():  : illegal State

I was not sure about what was happening, but I had the feeling that I was doing something wrong, because I would not expect that the client was dying in this scenario. When I was taking a look at the transaction page in the Geode documentation, I realized that in Java, the transaction example includes the call to the exists() method (link to the doc) before performing rollback.

if(txManager.exists()) {
    txManager.rollback();
}     

I added the call to the function to my C++ client and the problem was solved. After that the client was showing some exceptions when one of the server was killed, but the client was not dying. So I felt that calling to this exists()method was something to be done in transactions and that it was missing from the C++ documentation.

Copy link
Contributor

@pdxcodemonkey pdxcodemonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test that reproduces the problem, if you can. If it's too much work, file a separate Jira work item for the test and I'll approve/merge this. Thanks!

@alb3rtobr
Copy link
Contributor Author

Dont worry, I think its not needed to split this in two different tickets, I will try to write a test when I find time. Im moving this PR to draft in the meanwhile.

@alb3rtobr alb3rtobr marked this pull request as draft September 17, 2020 08:28
@alb3rtobr
Copy link
Contributor Author

There you are a test that shows the problem. I have divided the code in two commits. In the first one, the test fails with the following error:

$ ctest -R TransactionsTest -j1
Test project /home/alb3rtobr/CLionProjects/Nordix/geode-native/build/cppcache/integration/test
    Start 61: TransactionsTest.ExceptionWhenRollingBackTx
1/1 Test #61: TransactionsTest.ExceptionWhenRollingBackTx ...***Exception: Child aborted 21.41 sec

0% tests passed, 1 tests failed out of 1

Total Test time (real) =  23.72 sec

The following tests FAILED:
	 61 - TransactionsTest.ExceptionWhenRollingBackTx (Child aborted)
Errors while running CTest

But after changing the code to call transactionManager->exists() before executing the rollback (the second commit), the test case works fine.

@alb3rtobr alb3rtobr marked this pull request as ready for review October 1, 2020 07:31
@alb3rtobr
Copy link
Contributor Author

PR rebased to fix conflict with CMakeLists.txt

@codecov-commenter
Copy link

Codecov Report

Merging #648 into develop will increase coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #648      +/-   ##
===========================================
+ Coverage    73.95%   74.21%   +0.26%     
===========================================
  Files          644      644              
  Lines        51132    51132              
===========================================
+ Hits         37813    37948     +135     
+ Misses       13319    13184     -135     
Impacted Files Coverage Δ
cppcache/src/ThinClientStickyManager.cpp 77.88% <0.00%> (-10.58%) ⬇️
...test/testThinClientPoolExecuteHAFunctionPrSHOP.cpp 91.20% <0.00%> (-3.71%) ⬇️
cppcache/src/ClientMetadataService.cpp 64.98% <0.00%> (-0.92%) ⬇️
cppcache/src/ClientMetadata.cpp 65.16% <0.00%> (-0.57%) ⬇️
cppcache/src/ThinClientLocatorHelper.cpp 85.71% <0.00%> (-0.40%) ⬇️
cppcache/src/ExecutionImpl.cpp 67.69% <0.00%> (-0.39%) ⬇️
cppcache/src/TcrConnection.cpp 72.48% <0.00%> (-0.32%) ⬇️
cppcache/src/ThinClientRedundancyManager.cpp 75.62% <0.00%> (-0.32%) ⬇️
cppcache/src/TcrMessage.cpp 85.62% <0.00%> (+0.31%) ⬆️
cppcache/src/ThinClientPoolDM.cpp 75.48% <0.00%> (+0.35%) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4617f22...b824345. Read the comment docs.

@pdxcodemonkey pdxcodemonkey merged commit 36b500e into apache:develop Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants