keith-turner commented on code in PR #4296:
URL: https://github.com/apache/accumulo/pull/4296#discussion_r1502926521
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobPriorityQueue.java:
##########
@@ -130,6 +131,10 @@ public synchronized boolean add(TabletMetadata
tabletMetadata, Collection<Compac
CjpqKey cjqpKey = addJobToQueue(tabletMetadata, job);
if (cjqpKey != null) {
newEntries.add(cjqpKey);
+ } else {
+ // The priority for this job was lower than all other priorities and
not added
+ // In this case we will return true even though a subset of the jobs,
or none,
+ // were added
Review Comment:
> I was curious about your thoughts on this comment. Do we still want to
return true if nothing is added? Do we still want to return true if only a
subset of the jobs are added? If we are only returning a different value when
the queue is closed, maybe we should return this to return nothing (void) and
throw an IllegalStateException?
The code that calls that add method only cares if the queue was closed or
not as it trying to deal with an expected race condition. Since the race
condition is an expected situation would be nice to avoid using an exception
for flow control. The boolean return type is super confusing, so it would be
good to improve it somehow. The following is one possible way to make the code
more clear.
* Change CompactionJobPriorityQueue return type from boolean to int, where
it returns the number of things added.
* Add `boolean isClosed()` method to CompactionJobPriorityQueue
Change [this code in
CompactionJobQueues](https://github.com/apache/accumulo/blob/401237dba1198000ddbc7a2296edd0a683b3f5e4/server/manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobQueues.java#L157)
to the following.
```java
while(pq.add(tabletMetadata, jobs) == 0 && pq.isClosed()){
```
The above makes it more explicit in the code that it cares about the closed
status of the queue.
--
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]
