GEODE-10161: Remove unnecessary synchronization from RedisList #7579
Conversation
- Now, with the addition of versioning and synchronization at the higher-level methods, all the synchronization can be removed from the small, helper methods we have. - We still need to ensure that there is synchronization between toData and within methods that are mutating the list. - Inline various helper methods in RedisList as they are not adding any value. - Use AvailablePortHelper to set up the port that the crashing VM will use. Without this there is interference from other tests when running multiple tests concurrently (in the stressTest job). - Ran at least 1000 iterations of all list-related DUnit tests without any failures.
jdeppe-pivotal
requested review from
nonbinaryprogrammer,
ringles,
upthewaterspout,
DonalEvans and
dschneider-pivotal
as code owners
There was a problem hiding this comment.
Not directly related to this PR, but looking at AbstractRedisData.fromDelta(), we seem to be inconsistent about whether we synchronize before calling deserializeFrom() on the various Delta types. I don't know if it's a big deal, but seems like either all of them should be synchronizing or none of them should be, otherwise it's just arbitrary.
| @@ -265,7 +267,7 @@ public void lset(Region<RedisKey, RedisData> region, RedisKey key, int index, by | |||
| throw new RedisException(ERROR_INDEX_OUT_OF_RANGE); | |||
| } | |||
|
|
|||
| elementReplace(adjustedIndex, value); | |||
| elementList.set(adjustedIndex, value); | |||
There was a problem hiding this comment.
This call is not inside a synchronized block but modifies the backing list. Is there a possibility of issues here?
There was a problem hiding this comment.
Since we're not updating the version here this should be OK. Synchronization is implemented to allow atomic updates to both the version and the underlying structure. Additionally synchronization also prevents the possibility of ConcurrentModificationExceptions which this set call won't trigger since it's merely changing an existing value and not changing the structure (size) of the list.
|
@DonalEvans I created https://issues.apache.org/jira/browse/GEODE-10244 for the |
higher-level methods, all the synchronization can be removed from the
small, helper methods we have.
and within methods that are mutating the list.
value.
use. Without this there is interference from other tests when running
multiple tests concurrently (in the stressTest job).
any failures.
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?