keith-turner commented on code in PR #4980:
URL: https://github.com/apache/accumulo/pull/4980#discussion_r1802114732
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java:
##########
@@ -113,6 +122,9 @@ public boolean equals(Object o) {
private final AtomicLong dequeuedJobs;
private final ArrayDeque<CompletableFuture<CompactionJobQueues.MetaJob>>
futures;
private long futuresAdded = 0;
+ private final Map<KeyExtent,Timer> jobAges;
+ private final Supplier<CompactionJobPriorityQueueStats> jobQueueStats;
+ private final AtomicReference<io.micrometer.core.instrument.Timer>
jobQueueTimer;
Review Comment:
Could change this to wrap an optional and initially set it to empty(). That
would void creating an optional object each time this is used.
```suggestion
private final
AtomicReference<Optional<io.micrometer.core.instrument.Timer>> jobQueueTimer;
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java:
##########
@@ -307,11 +332,49 @@ private CjpqKey addJobToQueue(TabletMetadata
tabletMetadata, CompactionJob job)
var key = new CjpqKey(job);
jobQueue.put(key, new CompactionJobQueues.MetaJob(job, tabletMetadata));
+ jobAges.computeIfAbsent(tabletMetadata.getExtent(), e -> Timer.startNew());
Review Comment:
Instead of adding here
```suggestion
```
Could add at the following location. This will only be done once for the
tablet. Also can handle the edge case of the queue being full at that location
in the code.
```diff
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java
b/server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java
index e4e1f4ca02..b55172276e 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java
@@ -213,6 +213,9 @@ public class CompactionJobPriorityQueue {
if (!newEntries.isEmpty()) {
checkState(tabletJobs.put(tabletMetadata.getExtent(), new
TabletJobs(generation, newEntries))
== null);
+ jobAges.computeIfAbsent(tabletMetadata.getExtent(),
e->Timer.startNew());
+ }else{
+ jobAges.remove(tabletMetadata.getExtent());
}
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java:
##########
@@ -307,11 +332,49 @@ private CjpqKey addJobToQueue(TabletMetadata
tabletMetadata, CompactionJob job)
var key = new CjpqKey(job);
jobQueue.put(key, new CompactionJobQueues.MetaJob(job, tabletMetadata));
+ jobAges.computeIfAbsent(tabletMetadata.getExtent(), e -> Timer.startNew());
return key;
}
public synchronized void clear() {
jobQueue.clear();
tabletJobs.clear();
+ jobAges.clear();
+
+ // TODO, what do we do here for the jobQueueTimer, if anything?
Review Comment:
Looking at how this is used its attempting to clean up memory for queues
that are no longer used. This could be caused a by a change in config that
causes new jobs to no longer be added. Could change this method to
`clearIfInactive(Duration duration)` where it will only clear if nothing has
been added in the passed in duration. The impl could scan `jobAges.values()`
and find the min elapsed time and compare that to the passed in duration.
The code that calls this clear method calls it every 5 mins so we could call
`clearIfInactive(Duration.ofMinutes(10))`.
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java:
##########
@@ -138,6 +150,10 @@ public CompactionJobPriorityQueue(CompactorGroupId
groupId, int maxSize) {
this.rejectedJobs = new AtomicLong(0);
this.dequeuedJobs = new AtomicLong(0);
this.futures = new ArrayDeque<>();
+ this.jobAges = new ConcurrentHashMap<>();
Review Comment:
This could be a HashMap because all accesses to it should be inside a sync
block.
--
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]
