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-4168 Can't get json object stored as PDX using the new protocol #1209
Conversation
GEODE-4116 Can't get PDX objects using the new protocol Added a distributed test to ensure end-to-end handling of JSON documents is functioning correctly. For GEODE-4168 I changed the class-check from equals() to isAssignableFrom(). For GEODE-4116 I modified the Get and GetAll operation handlers to inhibit deserialization of PdxInstances when reading values from the cache. The test for 4116 ensures that the value is in serialized form by putting it into a distributed Region in another JVM. There are unrelated javadoc changes in this commit for DM.java and a couple of classes in the protobuf Driver module. I also added constraints to the Regions in the Driver's unit test to get rid of compilation warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -455,7 +455,10 @@ void addHostedLocators(InternalDistributedMember member, Collection<String> loca | |||
/** | |||
* returns the type of node | |||
* | |||
* @return | |||
* @see DistributionManager#NORMAL_DM_TYPE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition to the Java docs.
|
||
Assert.assertTrue(result instanceof Success); | ||
PdxInstance pdxInstance = (PdxInstance) getCache().getRegion(regionName).get(key); | ||
assertEquals("__GEMFIRE_JSON", pdxInstance.getClassName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a constant that could be used instead of the string literal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked for a constant but couldn't find one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// test hook | ||
@Override | ||
public void setReadSerialized(boolean value) { | ||
public void setReadSerializedForTest(boolean value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we deprecate setReadSerializedForTest and tell people not to use it? I can't really see a valid use case for changing this value on the fly. I do see some existing yucky tests that are trying to test 20 things without closing the cache that seem to be using this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a deprecation annotation
& GEODE-4116 Can't get PDX objects using the new protocol
Added a distributed test to ensure end-to-end handling of JSON documents
is functioning correctly. For GEODE-4168 I changed the class-check from
equals() to isAssignableFrom(). For GEODE-4116 I modified the Get and
GetAll operation handlers to inhibit deserialization of PdxInstances
when reading values from the cache. The test for 4116 ensures that
the value is in serialized form by putting it into a distributed Region
in another JVM.
There are unrelated javadoc changes in this commit for DM.java and
a couple of classes in the protobuf Driver module. I also added
constraints to the Regions in the Driver's unit test to get rid of
compilation warnings.
@galen-pivotal @PivotalSarge @WireBaron @upthewaterspout
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?
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.