GEODE-9019: Fixed synchronization blocks missed in previous commit #6123
Conversation
nabarunnag
requested review from
jdeppe-pivotal,
nonbinaryprogrammer and
sabbey37
as code owners
|
In a previous commit I missed out the synchronized block for toData in RedisHash and RedisSet, but then noticed that RedisString will also now require the synchronized block. |
| @@ -615,7 +615,7 @@ public int setbit( | |||
| */ | |||
|
|
|||
| @Override | |||
| public void toData(DataOutput out, SerializationContext context) throws IOException { | |||
| public synchronized void toData(DataOutput out, SerializationContext context) throws IOException { | |||
There was a problem hiding this comment.
I don't think it's just toData in for RedisString, I believe other methods need to be synchronized as well (like RedisHash and RedisSet do).
There was a problem hiding this comment.
Also, it would be nice to have a failing test for this if possible?
There was a problem hiding this comment.
- There are a couple of confirmSerializationIsStable tests and testSerializables. Is there any specific feature that you - like me to check in a test?
- Also, in this commit, I was just primarily undoing the removal of the synchronized blocks that I missed in the previous commit.
There was a problem hiding this comment.
I will create another ticket to figure out which other methods are not synchronized and fix those. This commit is just to undo the synchronizied commits I missed
There was a problem hiding this comment.
- There are a couple of confirmSerializationIsStable tests and testSerializables. Is there any specific feature that you - like me to check in a test?
- added tests
There was a problem hiding this comment.
Ok. I will approve this PR with the understanding that there will be another fix to synchronize the rest of the memthods in RedisString.
When I was talking about tests, I meant tests that show that we actually need synchronization. Like a test that shows data corruption if we don't have synchronization. I'm not sure there is much value in programatically testing that the method has a synchronization modifier.
nabarunnag
changed the title
nabarunnag
changed the title
There was a problem hiding this comment.
LGTM for replacing the synchronized blocks. We do have a test - CrashAndNoRepeatDUnitTest, that tests APPEND when systems are failing. It hasn't failed yet with issues so we'll probably need a test that exercises this class directly for concurrency issues. We'll probably at least need to synchronize append().
…pache#6123) * Added tests to validate synchronized modifiers. * Enum serialization fixed, comments updated (cherry picked from commit 0b9c6b1)
GEODE-9019: Serialization improvements in geode-redis (#6115) * DataSerializable classes were converted in DataSerializableFixedID * serialVersionID were added to the unavoidable Serializable classes GEODE-9019: Fixed synchronization blocks missed in previous commit (#6123) * Added tests to validate synchronized modifiers. * Enum serialization fixed, comments updated * Fixed the pom error - geode-membership in geode-redis (cherry picked from commit 7aae7b8) (cherry picked from commit 0b9c6b1)
…pache#6123) * Added tests to validate synchronized modifiers. * Enum serialization fixed, comments updated
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 buildrun 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, check Concourse 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.