-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52176/#review150094
-----------------------------------------------------------



Looks like a good fix for a really complicated issue!

I had a couple of minor nitpicks, below. Also, is there a reason why you did 
not remove the code to ((GatewaySenderEventImpl)object).makeHeapCopyIfOffHeap() 
in SerialGatewaySenderQueue.peek? It seems like since you moved the copy to 
peekAhead that code is not doing anything anymore?

Is there any way to write at least a unit test for this code?


geode-core/src/main/java/org/apache/geode/internal/cache/wan/serial/SerialGatewaySenderQueue.java
 (line 778)
<https://reviews.apache.org/r/52176/#comment217932>

    valueOf is uneccessary here.



geode-core/src/main/java/org/apache/geode/internal/cache/wan/serial/SerialGatewaySenderQueue.java
 (line 796)
<https://reviews.apache.org/r/52176/#comment217931>

    Minor nit - can't you just write this code like this?
    
    Long currentKey = getCurrentKey();
    if(currentKey == null) {
    
    Seems less confusing that way.


- Dan Smith


On Sept. 22, 2016, 9:46 p.m., nabarun nag wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52176/
> -----------------------------------------------------------
> 
> (Updated Sept. 22, 2016, 9:46 p.m.)
> 
> 
> Review request for geode, Barry Oglesby, Eric Shu, Jason Huynh, Dan Smith, 
> and xiaojian zhou.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Before my changes:
> 1. peek calls peekAhead to get the object from the sender queue to be put 
> into the batch for dispatch.
> 2. peekAhead gets the current key for which it is able to get an object back 
> by calling optimalGet.
> 3. It puts that current key into the peekedIds list and returns the object 
> back to peek.
> 4. peek then tries to make a heapcopy and only if it is successful it will 
> put the object into the dispatch batch.
> 5. Here is the issue, now conflation may kick in before peek is able to do 
> heapcopy and that object is removed from the queue and hence the object will 
> not be placed in the dispatch batch. However the the key representing that 
> object in the queue still exist in the PeekedIDs list.
> 6. So now there is an extra key in the peekedIds list.
> 7. Batch is dispatched and now the ack is received, so things need to be 
> removed from the sender queue.
> 8. remove() is called in a for loop with the count set to size of dispatch 
> batch for which ack was received. 
> 9. The remove() operation picks up keys from peekedIds list sequentially and 
> calls remove(key) on the queue.
> 10. Now since the batch size is smaller than the Ids peeked, element will 
> still linger behind after all remove calls are completed.
> 11. so tests which wait for queues to be empty will hang forever.
> 
> Solution:
> Since peek() doesnt have access to the current key but just the object and 
> hence cannot remove it from peekedIDs list, we moved the check for heapcopy 
> into peekAhead. 
> 
> So now only if a successful heap copy is made then only the key will be 
> placed into the peekedIDs list.
> 
> 
> Diffs
> -----
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/wan/serial/SerialGatewaySenderQueue.java
>  a8bb72d 
> 
> Diff: https://reviews.apache.org/r/52176/diff/
> 
> 
> Testing
> -------
> 
> precheck
> 
> 
> Thanks,
> 
> nabarun nag
> 
>

Reply via email to