> On Sept. 22, 2016, 11:02 p.m., Dan Smith wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/wan/serial/SerialGatewaySenderQueue.java, > > line 796 > > <https://reviews.apache.org/r/52176/diff/1/?file=1508581#file1508581line796> > > > > Minor nit - can't you just write this code like this? > > > > Long currentKey = getCurrentKey(); > > if(currentKey == null) { > > > > Seems less confusing that way. > > nabarun nag wrote: > I completely agree with this. I was following the way other if > comparisons were done in the peekAhead. -> > > while (before(currentKey, getTailKey()) > && (object = getObjectInSerialSenderQueue(currentKey)) == null) { > > I will fix such comparisons in other places too > > Dan Smith wrote: > Your example above is in a while loop, so it actually is fetching the > object on each iteration. > > nabarun nag wrote: > aah!! i assumed the amount of confusion would be same in the comparison > within an if condition and a while loop. Hence changed it to > object = getObjectInSerialSenderQueue(currentKey); > while (before(currentKey, getTailKey()) > && (object == null)) { > currentKey = inc(currentKey); > object = getObjectInSerialSenderQueue(currentKey); > } > > I assumed keeping both the functions incrementing current key and > fetching the object within the loop was a good idea. > > Do you think its not a good idea?
What you did looks good to me. - Dan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52176/#review150094 ----------------------------------------------------------- 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 > >