You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ddanielr (via GitHub)" <gi...@apache.org> on 2023/12/14 22:35:30 UTC
[PR] Handle exception when compaction queue is empty [accumulo]
ddanielr opened a new pull request, #4079:
URL: https://github.com/apache/accumulo/pull/4079
Handles a `NullElementException` that is thrown if `getLowestPriority` is called on an empty compaction priority queue.
This is expected to fix the flaky MetricsIT test as the metric `METRICS_COMPACTOR_JOB_PRIORITY_QUEUE_JOBS_PRIORITY` will always return a value for a given queue.
@keith-turner If a queue is empty, what should the queue priority return? Right now it's `0` but I'm not sure if that's the correct "empty" value we want to set.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Handle exception when compaction queue is empty [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr merged PR #4079:
URL: https://github.com/apache/accumulo/pull/4079
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Handle exception when compaction queue is empty [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4079:
URL: https://github.com/apache/accumulo/pull/4079#discussion_r1428615177
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java:
##########
@@ -158,6 +158,9 @@ public synchronized long getQueuedJobs() {
}
public synchronized long getLowestPriority() {
+ if (jobQueue.isEmpty()) {
Review Comment:
Each queue is registered with the MeterRegistry which calls this method directly on update.
So the Micrometer code would have to support handling an Optional.
Looking at their docs, I don't see anything popping out about Optionals.
We could certainly write some test cases and see what happens.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Handle exception when compaction queue is empty [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4079:
URL: https://github.com/apache/accumulo/pull/4079#discussion_r1430756432
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java:
##########
@@ -158,6 +158,9 @@ public synchronized long getQueuedJobs() {
}
public synchronized long getLowestPriority() {
+ if (jobQueue.isEmpty()) {
Review Comment:
> Looking at their docs, I don't see anything popping out about Optionals.
ok, that makes sense. Then if this method returned an optional then when its empty we would have to unregister the metrics, which would not be pretty.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Handle exception when compaction queue is empty [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4079:
URL: https://github.com/apache/accumulo/pull/4079#discussion_r1427955608
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java:
##########
@@ -158,7 +159,11 @@ public synchronized long getQueuedJobs() {
}
public synchronized long getLowestPriority() {
- return jobQueue.lastKey().job.getPriority();
+ CompactionJobPriorityQueue.CjpqKey highestJob = null;
+ try {
+ highestJob = jobQueue.lastKey();
+ } catch (NoSuchElementException e) {}
+ return highestJob == null ? 0 : highestJob.job.getPriority();
Review Comment:
If you get a `NoSuchElementException` here, then `highestJob` will always be `null`. I think you can rewrite this as:
```
try {
return jobQueue.lastKey().job.getPriority();
} catch (NoSuchElementException e) {
return 0;
}
```
The suggestion above relies on an Exception being raised, which is expensive. Another alternative would be:
```
if (jobQueue.isEmpty()) {
return 0;
}
return jobQueue.lastKey().job.getPriority();
```
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Handle exception when compaction queue is empty [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4079:
URL: https://github.com/apache/accumulo/pull/4079#discussion_r1428140159
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java:
##########
@@ -158,6 +158,9 @@ public synchronized long getQueuedJobs() {
}
public synchronized long getLowestPriority() {
+ if (jobQueue.isEmpty()) {
Review Comment:
If this method returned an OptionalLong, could the metrics code not emit a metric when its empty?
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Handle exception when compaction queue is empty [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #4079:
URL: https://github.com/apache/accumulo/pull/4079#issuecomment-1858035121
Merging this for now. I'll follow up with Keith later.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org