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-4375: Fix problem that an exception occurs when transaction from CacheServer via Pool #1363

Merged
merged 2 commits into from Jan 31, 2018

Conversation

masaki-yamakawa
Copy link
Contributor

CacheServer via Pool

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • [ N?A] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

LocalRegion region = getRegionByPath(msg.getDM(),
parentRegionPath != null ? parentRegionPath : regionPath);
boolean readShadowKey =
(region != null ? (region.getPoolName() == null) : !msg.getDM().isLoner());

Choose a reason for hiding this comment

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

These two lines with their ternaries are so dense with logic, that I wonder if it might be worthwhile to split this out into a private method and break this out more into variables and maybe even a if/else statement instead of the ternary to make this easier to follow.

@masaki-yamakawa
Copy link
Contributor Author

@pivotal-amurmann Thank you for your review.
I splitted the added code out into a private method and made it easy to understand.

@dschneider-pivotal dschneider-pivotal merged commit c6ce067 into apache:develop Jan 31, 2018
PivotalSarge pushed a commit to PivotalSarge/geode that referenced this pull request Jan 31, 2018
@bschuchardt
Copy link
Contributor

Good work on this PR.

For future reference, this is not the correct way to test for backward compatibility. The code in the old client is 1.5 code. Shoving 1.3 into Version.CURRENT is making the client pretend to be a 1.3 client and servers will serialize messages accordingly. A change in handshake serialization on my "develop" branch for 1.5 caused this test to throw an OutOfMemoryError and blew up my pre-checkin run.

The correct way to create an old-version client is to use Host.getVM(versionName, vmNumber), as in
Host.getHost(0).getVM("130", 4)

Then it will actually be running with 1.3 code.

https://cwiki.apache.org/confluence/display/GEODE/Writing+Backward-compatibility+DUnit+tests

I'm fixing up the test and will make it a parameterized test running against all available old-versions.

@masaki-yamakawa
Copy link
Contributor Author

@bschuchardt I'm sorry & Thank you for your future reference.
From now on, I will test backward compatibility with reference to Wiki's information.
BTW, do I need to fix the test class?

bschuchardt added a commit that referenced this pull request Feb 6, 2018
…m CacheServer via Pool (#1363)

Modifying the test for proper backward-compatibility testing
@masaki-yamakawa masaki-yamakawa deleted the feature/GEODE-4375 branch March 4, 2018 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants