GEODE-10323: Remove schedule threads in MemoryAllocatorImpl constructor #7715
Conversation
albertogpz
force-pushed
the
feature/GEODE-10323
branch
from
211c74f
to
d7fcb75
Compare
The scheduled executor used in MemoryAllocatorImpl was scheduled in the constructor. This provoked intermittent failures in OffHeapStorageJUnitTest testCreateOffHeapStorage test cases due to a race condition. The scheduling has been moved to a new method (start()) in the MemoryAllocatorImpl class that is in turn invoked in the create() static method.
albertogpz
force-pushed
the
feature/GEODE-10323
branch
from
d7fcb75
to
192c625
Compare
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java
Outdated
Show resolved
Hide resolved
albertogpz
force-pushed
the
feature/GEODE-10323
branch
2 times, most recently
from
9f6c8a4
to
a94f1d0
Compare
albertogpz
force-pushed
the
feature/GEODE-10323
branch
from
a94f1d0
to
183c329
Compare
|
I assigned https://issues.apache.org/jira/browse/GEODE-10323 to you since you're working on it. Thanks again! |
albertogpz
requested review from
pivotal-eshu and
dschneider-pivotal
as code owners
| OffHeapMemoryStats stats, int slabCount, long offHeapMemorySize, long maxSlabSize, | ||
| Slab[] slabs, SlabFactory slabFactory, int updateOffHeapStatsFrequencyMs) { | ||
| Slab[] slabs, SlabFactory slabFactory, | ||
| Supplier<Integer> updateOffHeapStatsFrequencyMsSupplier, |
There was a problem hiding this comment.
It seems like the update frequency "int" should be part of the NonRealTimeStatsUpdater and then you wouldn't need two suppliers.
There was a problem hiding this comment.
It's a good idea but there is one test case in which you don't pass a Supplier so that the default NonRealTimeStatsUpdater is constructed but you need to specify a smaller frequency so that the test does not need to wait for the default frequency value:
See testUpdateNonRealTimeOffHeapStorageStats() from class OffHeapStorageNonRuntimeStatsJUnitTest.
For this test case I would need to specify the frequency but not the supplier.
| } | ||
|
|
||
| void start() { | ||
| if (nonRealTimeStatsUpdater != null) { |
There was a problem hiding this comment.
In the constructor above it looks like you always initialize nonRealTimeStatsUpdaterSupplier to non-null. So what good are these null checks? Oh I see. Your supplier returns null. That works but I think it would be better to have the supplier returns a dummy NonRealTimeStatsUpdater that does nothing.
There was a problem hiding this comment.
I agree. As I was too quick to merge the PR, should I open a new one?
…or (#7715) * GEODE-10323: Remove schedule threads in MemoryAllocatorImpl The scheduled executor used in MemoryAllocatorImpl was scheduled in the constructor. This provoked intermittent failures in OffHeapStorageJUnitTest testCreateOffHeapStorage test cases due to a race condition. The scheduling has been moved to a new method (start()) in the MemoryAllocatorImpl class that is in turn invoked in the create() static method. * GEODE-10323: Extract update stats code to new class
The scheduled executor used in MemoryAllocatorImpl
was scheduled in the constructor. This provoked
intermittent failures in OffHeapStorageJUnitTest testCreateOffHeapStorage
test case due to a race condition.
The scheduling has been moved to a new method (start())
in the MemoryAllocatorImpl class that is in turn
invoked in the create() static method.
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?