kirklund commented on code in PR #7715:
URL: https://github.com/apache/geode/pull/7715#discussion_r879742177
##########
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java:
##########
@@ -66,7 +67,9 @@ public class MemoryAllocatorImpl implements MemoryAllocator {
private final ScheduledExecutorService updateNonRealTimeStatsExecutor;
- private final ScheduledFuture<?> updateNonRealTimeStatsFuture;
+ private volatile ScheduledFuture<?> updateNonRealTimeStatsFuture;
Review Comment:
The fix I described in the Jira ticket might be too simple. One overall
recommendation for newer Java code is to use atomics instead of raw volatile
fields. Then you can make it final: `final AtomicReference<Future<?>>`.
Also, I would be tempted to extract `updateNonRealTimeStatsExecutor` and
`updateNonRealTimeStatsFuture` together in a new class that
`MemoryAllocatorImpl` collaborates with. The smaller and more focused these
classes are, the easier it is to test each of them in isolation. A collaborator
gives you lots of options for mocking, verifying, etc. I can usually find a
class design (ie, composition of multiple classes) that resonates really well
and seems to address all of the issues I can find in the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
