Adds completable futures to compaction queue #4726
Conversation
Adds completeable futures to the queue of compaction jobs. This allows for async notification when something is added to the queue. The compaction queues code would drop queues that became empty. The concept of queues being empty became more complex with this change. A queue would be considered empty when there were no futures and the queue was empty. This increased complexity of empty would have made the code for dropping empty queues more complex. Instead of increasing the complexity of this code chose to drop removing empty queues. This means that if a compaction group is used and then no longer used that it will have a small empty datastructure sitting around in map for the process lifetime. That is unlikely to cause memory issues. Therefore decided the increased complexity was not worthwhile given it was unlikely to cause memory problems.
|
This change was made in support of #4715 |
cshannon
left a comment
cshannon
left a comment
There was a problem hiding this comment.
This LGTM so far, I plan to test it out later today against #4715 and see how it goes. I figure we can make changes if we discover something at that point.
For now it makes sense to keep both the sync and async methods for getting a job as the current code still uses that until #4715 is done, but I was wondering if you were planning to drop the sync entry point to get a job eventually? I was just curious if there was any reason to keep it and it might make the code simpler if we only support async at some point. You can still get sync behavior if desired by just waiting for the future to complete.
Yeah, it we can drop it once it becomes unused. Could possibly drop it in #4715, but do not have to. If we do not drop it in #4715 we need to remember to open an issue to drop it before merging #4715. I realized there is a problem with the new code for the case where nothing is ever added to the priority queue, get async is called repeatedly, and the futures are canceled. In this case the canceled futures will build up in memory. I think this easy to handle though, will open up a follow on PR to handle that case. |
2 participants
Adds completeable futures to the queue of compaction jobs. This allows for async notification when something is added to the queue.
The compaction queues code would drop queues that became empty. The concept of queues being empty became more complex with this change. A queue would be considered empty when there were no futures and the queue was empty. This increased complexity of empty would have made the code for dropping empty queues more complex. Instead of increasing the complexity of this code chose to drop removing empty queues. This means that if a compaction group is used and then no longer used that it will have a small empty datastructure sitting around in map for the process lifetime. That is unlikely to cause memory issues. Therefore decided the increased complexity was not worthwhile given it was unlikely to cause memory problems.