Re: HBASE-2646

[jeff whiting <je...@qualtrics.com>]

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 <je...@qualtrics.com> 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
> jeffw@qualtrics.com
> 
> 
> 
> 
> 
> 
> 
> 

--
Jeff Whiting
Qualtrics Labs Inc
jeffw@qualtrics.com

Re: HBASE-2646

[jeff whiting <je...@qualtrics.com>]

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 <je...@qualtrics.com> 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 <je...@qualtrics.com> 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
>> jeffw@qualtrics.com
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
> 
> --
> Jeff Whiting
> Qualtrics Labs Inc
> jeffw@qualtrics.com
> 
> 
> 
> 
> 
> 
> 
> 

--
Jeff Whiting
Qualtrics Labs Inc
jeffw@qualtrics.com