You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "jhuynh1 (GitHub)" <gi...@apache.org> on 2019/02/15 17:03:28 UTC
[GitHub] [geode] jhuynh1 opened pull request #3198: WIP: GEODE-6412:
Improve concurrency for getBucketIndex
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](http://www.apache.org/legal/resolved.html#category-a)?
### 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.
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #3198: GEODE-6412:
Improve concurrency for getBucketIndex
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
Modified test to use the asymmetric thread groups. Thanks for the suggestion/info!
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] boglesby commented on pull request #3198: WIP:
GEODE-6412: Improve concurrency for getBucketIndex
Posted by "boglesby (GitHub)" <gi...@apache.org>.
Any reason bucketIndexes can't just be a ConcurrentHashMap rather than a SynchronizedMap?
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #3198: WIP:
GEODE-6412: Improve concurrency for getBucketIndex
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
I assume the hope is that this catch will cover the case of an exception being thrown while iterating over the bucketIndexes synchronized map. The thing that makes me nervous about this is that the documentation for synchronized maps says:
" It is imperative that the user manually synchronize on the returned map when iterating over any of its collection views
...
**Failure to follow this advice may result in non-deterministic behavior.** "
https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#synchronizedMap%28java.util.Map%29
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #3198:
GEODE-6412: Improve concurrency for getBucketIndex
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Awesome!
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #3198: GEODE-6412:
Improve concurrency for getBucketIndex
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
This should now be fixed. Eagle eye ;-)
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #3198:
WIP: GEODE-6412: Improve concurrency for getBucketIndex
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Curious, why not use the JMH asymmetric thread group feature?
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #3198: WIP:
GEODE-6412: Improve concurrency for getBucketIndex
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
I'm not sure if non-deterministic behavior is just limited to the occasional ConcurrentModificationException.
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #3198: WIP:
GEODE-6412: Improve concurrency for getBucketIndex
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
We probably could change that as well.
In this case we still need to protect the returned value too (a list as the value)
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 closed pull request #3198: GEODE-6412:
Improve concurrency for getBucketIndex
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
[ pull request closed by jhuynh1 ]
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #3198:
WIP: GEODE-6412: Improve concurrency for getBucketIndex
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Spelling. `PartitiionedIndex` => `PartitionedIndex`
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #3198: WIP:
GEODE-6412: Improve concurrency for getBucketIndex
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
I would be interested in knowing what other non-deterministic behavior can be exhibited. Since this just wraps a basic HashMap, we can probably dig a bit and see what other exceptions are thrown...
For this specific usage, if the value no longer exists and we somehow retrieve it, that timing issue would exist anyways (imagine we retrieve it under a synchronize block but then soon after it's removed from the list). The end result would be an index that no longer is present in the map/list combo but returned to the caller.
This method gets a bit of leeway as it's just picking an arbitrary index from this collection, other than other exceptions being thrown, retrieving something out of order isn't a big deal for this specific method.
In either case, we can improve performance even more by caching the arbitrary index. So the non-determinism issue kind of goes away. for now... to be continued
[ Full content available at: https://github.com/apache/geode/pull/3198 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org