keith-turner commented on code in PR #3629:
URL: https://github.com/apache/accumulo/pull/3629#discussion_r1268882132
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -229,7 +229,8 @@ public ScanServer(ConfigOpts opts, String[] args) {
"Tablet metadata caching less than one minute, may cause excessive
scans on metadata table.");
}
tabletMetadataCache =
- Caffeine.newBuilder().expireAfterWrite(cacheExpiration,
TimeUnit.MILLISECONDS)
+
context.getCaches().createNewBuilder(CacheName.SCAN_SERVER_TABLET_METADATA,
true)
Review Comment:
This will be nice to have metrics about.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/BulkImport.java:
##########
@@ -362,13 +362,15 @@ private static Map<String,Long>
getFileLenMap(List<FileStatus> statuses) {
return fileLens;
}
- private static Cache<String,Long> getPopulatedFileLenCache(Path dir,
List<FileStatus> statuses) {
+ private static Cache<String,Long> getPopulatedFileLenCache(ClientContext
ctx, Path dir,
+ List<FileStatus> statuses) {
Map<String,Long> fileLens = getFileLenMap(statuses);
Map<String,Long> absFileLens = new HashMap<>();
fileLens.forEach((k, v) -> absFileLens.put(pathToCacheId(new Path(dir,
k)), v));
- Cache<String,Long> fileLenCache = Caffeine.newBuilder().build();
+ Cache<String,Long> fileLenCache =
Review Comment:
This is also a short lived cache, I think its created for each bulk v2
client request that inspects files. If that is a problem could not emit
metrics.
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionJobGenerator.java:
##########
@@ -65,7 +66,8 @@ public CompactionJobGenerator(PluginEnvironment env,
serviceIds =
servicesConfig.getPlanners().keySet().stream().map(CompactionServiceId::of)
.collect(Collectors.toUnmodifiableSet());
- dispatchers = Caffeine.newBuilder().maximumSize(10).build();
+ dispatchers =
Caches.getInstance().createNewBuilder(CacheName.COMPACTION_DISPATCHERS, true)
Review Comment:
This cache is short lived, maybe it should not emit metrics? Also there
could be multiple instance of this object in multiple processes (like the
manager and tserver via TabletMgtmIter), will that cause problems for metrics?
##########
core/src/main/java/org/apache/accumulo/core/util/cache/Caches.java:
##########
@@ -75,22 +78,26 @@ public void registerMetrics(MeterRegistry registry) {
this.registry = registry;
}
- private void setupMicrometerMetrics(Caffeine<Object,Object> cacheBuilder,
String name) {
+ private boolean setupMicrometerMetrics(Caffeine<Object,Object> cacheBuilder,
String name) {
if (registry != null) {
try {
cacheBuilder.recordStats(
() -> new CaffeineStatsCounter(registry, name,
MetricsUtil.getCommonTags()));
+ return true;
} catch (IllegalStateException e) {
// recordStats was already called by the cacheBuilder.
Review Comment:
Would be good to log this in case its not the expected case you want to
ignore. Could log it at trace.
--
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]
