keith-turner commented on code in PR #4855:
URL: https://github.com/apache/accumulo/pull/4855#discussion_r1747290324
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/memory/LargestFirstMemoryManager.java:
##########
@@ -60,8 +57,7 @@ public class LargestFirstMemoryManager {
private double compactionThreshold;
private long maxObserved;
private final HashMap<TableId,Long> mincIdleThresholds = new HashMap<>();
- private final Cache<TableId,Long> mincAgeThresholds =
- Caffeine.newBuilder().expireAfterWrite(5, TimeUnit.MINUTES).build();
+ private final HashMap<TableId,Long> mincAgeThresholds = new HashMap<>();
Review Comment:
Removing this bit of caching would simplify this code. The map in this case
is very short lived, so I don't think it adds latency in this case however it
does add complexity to the code that makes it harder to maintain. Looking at
the way this code works the main benefit of this little map is that it avoids
reparsing the duration for each tablet of the same table. Without the map we
would keep converting at string like `5m` to milliseconds over and over as we
examine all the tablets in the tablet server. However instead of doing that
outside of the configuration in some places we should probably push that down
into the configuration and cache the result of parsing `5m` inside the
configuration objs until the configuration changes. If we cached the parsing
inside the cache that could make #4742 more performant where it caches Duration
objects and does not create them on each call.
For this PR we could just remove the map its using to avoid reparsing and
open an issue about caching duration parsing inside the cache.
--
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]
