----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52176/#review150325 -----------------------------------------------------------
Ship it! A good fix with very clean description. - xiaojian zhou On Sept. 23, 2016, 9:58 p.m., nabarun nag wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52176/ > ----------------------------------------------------------- > > (Updated Sept. 23, 2016, 9:58 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 > >