That is an astute observation. Stepping through the code with the threads stopping execution at the points in code you suggest would indeed make it so take() would return the lower priority compactionRequest, remove the higher priority compaction request from regionsInQueue, and finally the add() method would complete and add the higher priority compaction onto the queue with no corresponding entry in the regionsInQueue hash (this is bad). Even if I move the queue.remove(queuedRequest) into the synchronized(regionsInQueue) we will run into the same problem.
Fortunately the worst thing that can happen is there is a request that doesn't have an entry in regionsInQueue that will eventually be executed with no adverse problem for the system other than extra work. This wont actually cause any problems to the system but PriorityCompactionQueue will have an inconsistent state which should be fixed. An immediate solution is not jumping out at me. So I need to think through the problem and see if I can't come up with a way to prevent the inconsistency. Thanks, ~Jeff p.s. I think this is a good discussion to have on the mailing list so I copying this email there. On Sep 28, 2010, at 11:15 AM, Ted Yu wrote: > Jeff: > In PriorityCompactionQueue.addToRegionsInQueue(), I noticed the following > call which is not synchronized: > queue.remove(queuedRequest); > > Now suppose before the above is executed, PriorityCompactionQueue.take() is > called. So queuedRequest is returned to the caller of take(). Later, this > line in take(): > removeFromRegionsInQueue(cr.getHRegion()); > would remove the newly added, higher priority request from regionsInQueue. > > Please comment. > > On Fri, Sep 24, 2010 at 11:32 AM, jeff whiting <[email protected]> wrote: > I'm sure it could. I haven't given it a test. My gut feeling is that will > work. They haven't really changed much of that in the 0.20 branch. > > ~Jeff > > On Sep 24, 2010, at 11:50 AM, Ted Yu wrote: > >> Jeff: >> Can your prioritycompactionqueue-0.20.4.patch be applied to 0.20.6 ? >> >> Thanks > > -- > Jeff Whiting > Qualtrics Labs Inc > [email protected] > > > > > > > > -- Jeff Whiting Qualtrics Labs Inc [email protected]
