Re: Review Request 60513: GEODE-3147 Set TLCommBuffer threadLocal for threads executing TXSynchronization

[Darrel Schneider]

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




geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
Lines 1796 (patched)


The current code is good but I had one more idea.
Why not have the "isSelector()" checks in setTLCommBuffer and 
releaseTLCommBuffer?
You can implement them to have the first line:
if (!isSelector()) return;

That would allow you to get rid of your call if isSelector in 
TXSynchronizationCommand
and your run method in TXSynchronizationRunnable would no longer need to do 
the null check.


- Darrel Schneider


On June 29, 2017, 11:39 a.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60513/
> ---
> 
> (Updated June 29, 2017, 11:39 a.m.)
> 
> 
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn 
> Gallinat.
> 
> 
> Bugs: GEODE-3147
> https://issues.apache.org/jira/browse/GEODE-3147
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Set byte buffer for the threads that need to send reply to clients when 
> Max_Threads are set.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/TXSynchronizationRunnable.java
>  35b0e75 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
>  472af09 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CommBufferPool.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
>  ebc9dab 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
>  b1b0cfb 
>   
> geode-core/src/test/java/org/apache/geode/internal/jta/ClientServerJTADUnitTest.java
>  51ef44a 
> 
> 
> Diff: https://reviews.apache.org/r/60513/diff/3/
> 
> 
> Testing
> ---
> 
> precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>

Re: Review Request 60513: GEODE-3147 Set TLCommBuffer threadLocal for threads executing TXSynchronization

[Eric Shu]

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

(Updated June 29, 2017, 6:39 p.m.)


Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn 
Gallinat.


Changes
---

Added a new interface based on review comment.


Bugs: GEODE-3147
https://issues.apache.org/jira/browse/GEODE-3147


Repository: geode


Description
---

Set byte buffer for the threads that need to send reply to clients when 
Max_Threads are set.


Diffs (updated)
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/TXSynchronizationRunnable.java
 35b0e75 
  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
 472af09 
  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CommBufferPool.java
 PRE-CREATION 
  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
 ebc9dab 
  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
 b1b0cfb 
  
geode-core/src/test/java/org/apache/geode/internal/jta/ClientServerJTADUnitTest.java
 51ef44a 


Diff: https://reviews.apache.org/r/60513/diff/3/

Changes: https://reviews.apache.org/r/60513/diff/2-3/


Testing
---

precheckin.


Thanks,

Eric Shu

Re: Review Request 60513: GEODE-3147 Set TLCommBuffer threadLocal for threads executing TXSynchronization

[Darrel Schneider]

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




geode-core/src/main/java/org/apache/geode/internal/cache/TXSynchronizationRunnable.java
Lines 42 (patched)


make "acceptor" final



geode-core/src/main/java/org/apache/geode/internal/cache/TXSynchronizationRunnable.java
Line 42 (original), 44 (patched)


I think it would be nice if you introduced a new interface named 
"CommBufferPool" that just has two methods on it (takeCommBuffer and 
releaseCommBuffer).
Change AcceptorImpl to extend this new interface and change this 
constructor to take a CommBufferPool instead of an AcceptorImpl.



geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
Line 145 (original), 145 (patched)


I think this would be easier to understand:
AcceptorImpl acceptor = serverConnection.getAcceptor();
if (!acceptor.isSelector()) {
  acceptor = null;
}
new TXSynchronizationRunnable(beforeCompletion, acceptor);


- Darrel Schneider


On June 28, 2017, 5:20 p.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60513/
> ---
> 
> (Updated June 28, 2017, 5:20 p.m.)
> 
> 
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn 
> Gallinat.
> 
> 
> Bugs: GEODE-3147
> https://issues.apache.org/jira/browse/GEODE-3147
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Set byte buffer for the threads that need to send reply to clients when 
> Max_Threads are set.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/TXSynchronizationRunnable.java
>  35b0e75 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
>  b1b0cfb 
>   
> geode-core/src/test/java/org/apache/geode/internal/jta/ClientServerJTADUnitTest.java
>  51ef44a 
> 
> 
> Diff: https://reviews.apache.org/r/60513/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>

Re: Review Request 60513: GEODE-3147 Set TLCommBuffer threadLocal for threads executing TXSynchronization

[Eric Shu]

> On June 28, 2017, 11:09 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
> > Lines 223 (patched)
> > 
> >
> > How about moving all these changes to TXSynchronizationRunnable?
> > When you construct it give it the serverConnection and then it can have 
> > needTLCommBuffer and do the setTLCommBuffer and releaseCommBuffer.
> > 
> > That way you do not need to make a bunch of changes to this class and 
> > it will be much easier to review.

Thanks for the suggestion, the new fix only acquire and release the comm buffer 
once now.


- Eric


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


On June 29, 2017, 12:20 a.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60513/
> ---
> 
> (Updated June 29, 2017, 12:20 a.m.)
> 
> 
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn 
> Gallinat.
> 
> 
> Bugs: GEODE-3147
> https://issues.apache.org/jira/browse/GEODE-3147
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Set byte buffer for the threads that need to send reply to clients when 
> Max_Threads are set.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/TXSynchronizationRunnable.java
>  35b0e75 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
>  b1b0cfb 
>   
> geode-core/src/test/java/org/apache/geode/internal/jta/ClientServerJTADUnitTest.java
>  51ef44a 
> 
> 
> Diff: https://reviews.apache.org/r/60513/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>

Re: Review Request 60513: GEODE-3147 Set TLCommBuffer threadLocal for threads executing TXSynchronization

[Eric Shu]

> On June 28, 2017, 7:31 p.m., anilkumar gingade wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
> > Lines 106 (patched)
> > 
> >
> > Can we move this to doBeforeCompletion() as done in 
> > createAfterCompletionRunnable() which returns the runnable.

Only need to set condition once for the two synchronization call now with 
Darrel's review.


- Eric


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


On June 29, 2017, 12:20 a.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60513/
> ---
> 
> (Updated June 29, 2017, 12:20 a.m.)
> 
> 
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn 
> Gallinat.
> 
> 
> Bugs: GEODE-3147
> https://issues.apache.org/jira/browse/GEODE-3147
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Set byte buffer for the threads that need to send reply to clients when 
> Max_Threads are set.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/TXSynchronizationRunnable.java
>  35b0e75 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
>  b1b0cfb 
>   
> geode-core/src/test/java/org/apache/geode/internal/jta/ClientServerJTADUnitTest.java
>  51ef44a 
> 
> 
> Diff: https://reviews.apache.org/r/60513/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>

Re: Review Request 60513: GEODE-3147 Set TLCommBuffer threadLocal for threads executing TXSynchronization

[Eric Shu]

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

(Updated June 29, 2017, 12:20 a.m.)


Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn 
Gallinat.


Changes
---

Fix review comments


Bugs: GEODE-3147
https://issues.apache.org/jira/browse/GEODE-3147


Repository: geode


Description
---

Set byte buffer for the threads that need to send reply to clients when 
Max_Threads are set.


Diffs (updated)
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/TXSynchronizationRunnable.java
 35b0e75 
  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
 b1b0cfb 
  
geode-core/src/test/java/org/apache/geode/internal/jta/ClientServerJTADUnitTest.java
 51ef44a 


Diff: https://reviews.apache.org/r/60513/diff/2/

Changes: https://reviews.apache.org/r/60513/diff/1-2/


Testing
---

precheckin.


Thanks,

Eric Shu

Re: Review Request 60513: GEODE-3147 Set TLCommBuffer threadLocal for threads executing TXSynchronization

[Darrel Schneider]

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




geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
Lines 223 (patched)


How about moving all these changes to TXSynchronizationRunnable?
When you construct it give it the serverConnection and then it can have 
needTLCommBuffer and do the setTLCommBuffer and releaseCommBuffer.

That way you do not need to make a bunch of changes to this class and it 
will be much easier to review.


- Darrel Schneider


On June 28, 2017, 11:58 a.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60513/
> ---
> 
> (Updated June 28, 2017, 11:58 a.m.)
> 
> 
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn 
> Gallinat.
> 
> 
> Bugs: GEODE-3147
> https://issues.apache.org/jira/browse/GEODE-3147
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Set byte buffer for the threads that need to send reply to clients when 
> Max_Threads are set.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
>  b1b0cfb 
>   
> geode-core/src/test/java/org/apache/geode/internal/jta/ClientServerJTADUnitTest.java
>  51ef44a 
> 
> 
> Diff: https://reviews.apache.org/r/60513/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>

Re: Review Request 60513: GEODE-3147 Set TLCommBuffer threadLocal for threads executing TXSynchronization

[anilkumar gingade]

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




geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
Lines 106 (patched)


Can we move this to doBeforeCompletion() as done in 
createAfterCompletionRunnable() which returns the runnable.


- anilkumar gingade


On June 28, 2017, 6:58 p.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60513/
> ---
> 
> (Updated June 28, 2017, 6:58 p.m.)
> 
> 
> Review request for geode, anilkumar gingade, Darrel Schneider, and Lynn 
> Gallinat.
> 
> 
> Bugs: GEODE-3147
> https://issues.apache.org/jira/browse/GEODE-3147
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Set byte buffer for the threads that need to send reply to clients when 
> Max_Threads are set.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
>  b1b0cfb 
>   
> geode-core/src/test/java/org/apache/geode/internal/jta/ClientServerJTADUnitTest.java
>  51ef44a 
> 
> 
> Diff: https://reviews.apache.org/r/60513/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>