I was thinking along the same lines. Adding an additional synchronization didn't seem like the right approach. So if we make sure we are taking off what we are expecting to then there wont be a problem.
~Jeff On Sep 28, 2010, at 2:41 PM, Ted Yu wrote: > Except for remove(Object r), all callers of removeFromRegionsInQueue() have > CompactionRequest information. > So CompactionRequest, cr, can be passed to removeFromRegionsInQueue() so that > we can perform some sanity check. > If cr.getPriority() is lower than the priority of the CompactionRequest > currently in regionsInQueue, removeFromRegionsInQueue() can return null which > indicates inconsistency. > The caller can discard cr upon seeing null from removeFromRegionsInQueue() > and try to get the next request from queue. > > The above avoids introducing another synchronization between accesses to > queue and regionsInQueue. > > My two cents. > > On Tue, Sep 28, 2010 at 11:45 AM, jeff whiting <[email protected]> wrote: > 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] > > > > > > > > -- Jeff Whiting Qualtrics Labs Inc [email protected]
