Re: Review Request 52176: GEODE-1926: Modification of peedAhead() function check if heapCopy is successful before adding the key to peekedIds

2016-09-24 Thread xiaojian zhou 

---
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
> 
>

Re: Review Request 52176: GEODE-1926: Modification of peedAhead() function check if heapCopy is successful before adding the key to peekedIds

2016-09-23 Thread Dan Smith 

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


Ship it!




Ship It!

- Dan Smith


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
> 
>

Re: Review Request 52176: GEODE-1926: Modification of peedAhead() function check if heapCopy is successful before adding the key to peekedIds

2016-09-23 Thread Dan Smith 


> 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
> > 
> >
> > 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
> 
>

Re: Review Request 52176: GEODE-1926: Modification of peedAhead() function check if heapCopy is successful before adding the key to peekedIds

2016-09-23 Thread nabarun nag 


> 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
> > 
> >
> > 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
> 
>

Re: Review Request 52176: GEODE-1926: Modification of peedAhead() function check if heapCopy is successful before adding the key to peekedIds

2016-09-23 Thread nabarun nag 

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



I was able to find paralleWANConflationDUnitTest but no 
serialWANConflationDUnitTest in Geode, 
Am I missing something or are serial WAN conflation tests under a different 
class.

- nabarun nag


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
> 
>

Re: Review Request 52176: GEODE-1926: Modification of peedAhead() function check if heapCopy is successful before adding the key to peekedIds

2016-09-23 Thread nabarun nag 

---
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 (updated)
-

  
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

Re: Review Request 52176: GEODE-1926: Modification of peedAhead() function check if heapCopy is successful before adding the key to peekedIds

2016-09-23 Thread Dan Smith 


> 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
> > 
> >
> > 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

Your example above is in a while loop, so it actually is fetching the object on 
each iteration.


- Dan


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


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
> 
>

Re: Review Request 52176: GEODE-1926: Modification of peedAhead() function check if heapCopy is successful before adding the key to peekedIds

2016-09-23 Thread nabarun nag 


> On Sept. 23, 2016, 8:29 p.m., Jason Huynh wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/wan/serial/SerialGatewaySenderQueue.java,
> >  line 796
> > 
> >
> > +1 on the less confusing part

sorry about that  copied previous coding style 
(object = optimalGet(currentKey)) == null)


- nabarun


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


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
> 
>

Re: Review Request 52176: GEODE-1926: Modification of peedAhead() function check if heapCopy is successful before adding the key to peekedIds

2016-09-23 Thread nabarun nag 


> On Sept. 22, 2016, 11:02 p.m., Dan Smith wrote:
> > 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?

was updated in a new patch.
review went on an older patch. sorry about that


I assumed peekAhead had already some JUnit tests written for it hence I was 
confident after pre-check and other WANtests passed. I will write few Junit 
tests if they are not present.


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

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


> 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 778
> > 
> >
> > valueOf is uneccessary here.

was updated in a new patch.
review went on an older patch. sorry about that


- nabarun


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


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
> 
>

Re: Review Request 52176: GEODE-1926: Modification of peedAhead() function check if heapCopy is successful before adding the key to peekedIds

2016-09-23 Thread Jason Huynh 

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



Not sure what an appropriate test would be for this, maybe create a dunit with 
test hooks or perhaps target the peekAhead method directly and override the 
optimalGet and makeHeapCopy methods to force/fake it down the problematic 
path... It would be nice to get a test if possible


geode-core/src/main/java/org/apache/geode/internal/cache/wan/serial/SerialGatewaySenderQueue.java
 (line 796)


+1 on the less confusing part


- Jason Huynh


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
> 
>