You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@geode.apache.org by "Kirk Lund (Jira)" <ji...@apache.org> on 2022/05/19 23:23:00 UTC
[jira] [Comment Edited] (GEODE-10323) OffHeapStorageJUnitTest testCreateOffHeapStorage fails with AssertionError: expected:<100> but was:<1048576>
[ https://issues.apache.org/jira/browse/GEODE-10323?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17539849#comment-17539849 ]
Kirk Lund edited comment on GEODE-10323 at 5/19/22 11:22 PM:
-------------------------------------------------------------
Here's our best recommendation for a fix.
Move the scheduling of {{updateNonRealTimeStatsFuture}} out of the constructor for {{MemoryAllocatorImpl}}:
{noformat}
updateNonRealTimeStatsFuture =
updateNonRealTimeStatsExecutor.scheduleAtFixedRate(freeList::updateNonRealTimeStats, 0,
updateOffHeapStatsFrequencyMs, TimeUnit.MILLISECONDS);
{noformat}
To a new method called {{start()}} or something similar:
{noformat}
void start() {
updateNonRealTimeStatsFuture =
updateNonRealTimeStatsExecutor.scheduleAtFixedRate(freeList::updateNonRealTimeStats, 0,
updateOffHeapStatsFrequencyMs, TimeUnit.MILLISECONDS);
}
{noformat}
Add a call to {{start()}} near just before returning the newly created {{MemoryAllocatorImpl}} from the static factory method {{private static MemoryAllocatorImpl create(...)}}.
{noformat}
result.start();
return result;
}
{noformat}
The newer tests introduced by PR #7407 should then use one of the static factory methods that invokes {{start()}} near the end just before returning the new instance of {{MemoryAllocatorImpl}}.
{{MemoryAllocatorImpl}} has acquired too many static factory methods including two named {{createForUnitTest}}. The use of methods "ForUnitTest" is a bad practice, but the more concerning problem here is that these static factory methods have a lot of duplicated code between them and they don't funnel through each other.
Tests that need to inject dependencies or different values should call into a static factory method is also being called by the product's code path. The test can then use the same code and provide different dependencies or different values while still using the same code path. Use of {{java.util.function.Supplier<T>}} for dependencies that may need it can also help make things more flexible for testing.
was (Author: klund):
Here's the best recommendation for a fix.
Move the scheduling of {{updateNonRealTimeStatsFuture}} out of the constructor for MemoryAllocatorImpl:
{noformat}
updateNonRealTimeStatsFuture =
updateNonRealTimeStatsExecutor.scheduleAtFixedRate(freeList::updateNonRealTimeStats, 0,
updateOffHeapStatsFrequencyMs, TimeUnit.MILLISECONDS);
{noformat}
To a new method called {{start()}} or something similar:
{noformat}
void start() {
updateNonRealTimeStatsFuture =
updateNonRealTimeStatsExecutor.scheduleAtFixedRate(freeList::updateNonRealTimeStats, 0,
updateOffHeapStatsFrequencyMs, TimeUnit.MILLISECONDS);
}
{noformat}
Add a call to {{start()}} near just before returning the newly created {{MemoryAllocatorImpl}} from the static factory method {{private static MemoryAllocatorImpl create(...)}}.
{noformat}
result.start();
return result;
}
{noformat}
The newer tests introduced by PR #7407 should then use one of the static factory methods that invokes {{start()}} near the end just before returning the new instance of {{MemoryAllocatorImpl}}.
{{MemoryAllocatorImpl}} has acquired too many static factory methods including two named {{createForUnitTest}}. The use of methods "ForUnitTest" is a bad practice, but the more concerning problem here is that these static factory methods have a lot of duplicated code between them and they don't funnel through each other.
Tests that need to inject dependencies or different values should call into a static factory method is also being called by the product's code path. The test can then use the same code and provide different dependencies or different values while still using the same code path. Use of {{java.util.function.Supplier<T>}} for dependencies that may need it can also help make things more flexible for testing.
> OffHeapStorageJUnitTest testCreateOffHeapStorage fails with AssertionError: expected:<100> but was:<1048576>
> ------------------------------------------------------------------------------------------------------------
>
> Key: GEODE-10323
> URL: https://issues.apache.org/jira/browse/GEODE-10323
> Project: Geode
> Issue Type: Bug
> Components: offheap
> Reporter: Kirk Lund
> Priority: Major
> Labels: needsTriage
>
> {{OffHeapStorageJUnitTest testCreateOffHeapStorage}} started failing intermittently due to commit a350ed2 which went in as "[GEODE-10087|https://issues.apache.org/jira/browse/GEODE-10087]: Enhance off-heap fragmentation visibility. ([#7407|https://github.com/apache/geode/pull/7407/commits/1640effc5c760afa8d9ec4c2743950bb1de0ef8f])".
> The failure stack looks like:
> {noformat}
> OffHeapStorageJUnitTest > testCreateOffHeapStorage FAILED
> java.lang.AssertionError: expected:<100> but was:<1048576>
> at org.junit.Assert.fail(Assert.java:89)
> at org.junit.Assert.failNotEquals(Assert.java:835)
> at org.junit.Assert.assertEquals(Assert.java:647)
> at org.junit.Assert.assertEquals(Assert.java:633)
> at org.apache.geode.internal.offheap.OffHeapStorageJUnitTest.testCreateOffHeapStorage(OffHeapStorageJUnitTest.java:220)
> {noformat}
> The cause is in {{MemoryAllocatorImpl}}. A new scheduled executor {{updateNonRealTimeStatsExecutor}} was added near the bottom of the constructor which immediately gets scheduled to invoke {{freeList::updateNonRealTimeStats}} repeatedly at the specified frequency of {{updateOffHeapStatsFrequencyMs}} whenever an instance of {{MemoryAllocatorImpl}} is created.
> {{freeList::updateNonRealTimeStats}} then updates {{setLargestFragment}} and {{setFreedChunks}}.
> Scheduling or starting any sort of threads from a constructor is considered a bad practice and should only be done via some sort of activation method such as {{start()}} which can be invoked from the static factory method for {{MemoryAllocatorImpl}}. The unit tests would then continue to construct instances via a constructor that does NOT invoke {{start()}}.
> {{OffHeapStorageJUnitTest testCreateOffHeapStorage}}, and possibly other tests, is written with the assumption that no other thread or component can or will be updating the {{OffHeapStats}}. Hence it is then safe for the test to setLargestFragment and then assert that it has the value passed in:
> {noformat}
> 219: stats.setLargestFragment(100);
> 220: assertEquals(100, stats.getLargestFragment());
> {noformat}
> Line 220 is the source of the assertion failure because {{updateNonRealTimeStatsExecutor}} is racing with the JUnit thread and changes the value of {{setLargestFragment}} immediately after the test sets the value to 100, but before the test invokes the assertion.
> Looking back at the [PR test results|https://cio.hdb.gemfire-ci.info/commits/pr/7407/junit?days=90] we can see that stress-new-test failed 5 times with this exact failure before being merged to develop:
> * http://files.apachegeode-ci.info/builds/apache-develop-pr/geode-pr-7407/test-results/repeatTest/1646140652/
> * http://files.apachegeode-ci.info/builds/apache-develop-pr/geode-pr-7407/test-results/repeatTest/1646154134/ (x2)
> * http://files.apachegeode-ci.info/builds/apache-develop-pr/geode-pr-7407/test-results/repeatTest/1646156997/
> * http://files.apachegeode-ci.info/builds/apache-develop-pr/geode-pr-7407/test-results/repeatTest/1646170346/
--
This message was sent by Atlassian Jira
(v8.20.7#820007)