GEODE-4712 GEODE-5943: shut down the bucketSorter when destroying the partitioned region#2845
GEODE-4712 GEODE-5943: shut down the bucketSorter when destroying the partitioned region#2845jinmeiliao merged 4 commits intoapache:developfrom
Conversation
kirklund
left a comment
kirklund
left a comment
There was a problem hiding this comment.
It's not clear to me why it would be necessary to bounce VMs or what the underlying flakiness is. I think the underlying cause of flakiness should be found and addressed rather than just resorting to bouncing the VMs which seems overly heavy handed to me.
kirklund
left a comment
kirklund
left a comment
There was a problem hiding this comment.
I think bouncing of VMs should be reserved for a very few special case tests involving forced disconnect or reconnect.
|
@kirklund I talked to Eric about this. He thinks bouncing the VM should be the approach here. But I think there may be other ways as well. maybe we can pair on this to find a better approach. I know why this happened, but I am not sure what's the correct way to fix the flakiness. |
|
Yesterday, Jinmei and I discovered that the JVM which goes critical and causes the failure has 12 GemFireCacheImpl instances in it. So this test seems to be hitting some sort of memory leak that needs to be fixed (not sure if it's a test or product leak). |
…PartitionRegion to release the reference to cache. * the bucketSorter thread is keeping the cache from being garbage collected after cache.close is called.
jinmeiliao
changed the title
| colocatedWithRegion.getColocatedByList().remove(this); | ||
| } | ||
|
|
||
| bucketSorter.shutdown(); |
There was a problem hiding this comment.
Needs null check. Its created if the eviction is configured.
dschneider-pivotal
left a comment
dschneider-pivotal
left a comment
There was a problem hiding this comment.
It would be great if you could add a unit test that showed the
| && this.getEvictionAttributes().getAlgorithm().isLRUHeap()) { | ||
| this.sortedBuckets = new ArrayList<BucketRegion>(); | ||
| this.bucketSorter = LoggingExecutors.newScheduledThreadPool("BucketSorterThread", 1); | ||
| this.bucketSorter = LoggingExecutors.newScheduledThreadPool("BucketSorterRunnable", 1); |
There was a problem hiding this comment.
This one should not be "BucketSorterRunnable". This is the thread name so the original "BucketSorterThread" was fine.
There was a problem hiding this comment.
I unit test would be great but not required.
There was a problem hiding this comment.
added an integration test and renamed the thread
4 participants
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, 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.