> 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.
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?
- nabarun
-----------------------------------------------------------
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
>
>
