[GitHub] [geode] kirklund commented on a diff in pull request #7715: GEODE-10323: Remove schedule threads in MemoryAllocatorImpl constructor

[GitBox <gi...@apache.org>]

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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org