> On Sept. 22, 2016, 9:07 p.m., Dan Smith wrote:
> > geode-core/src/test/java/org/apache/geode/test/dunit/standalone/ProcessManager.java,
> >  line 171
> > <https://reviews.apache.org/r/52172/diff/1/?file=1508461#file1508461line171>
> >
> >     What is this for?
> 
> Bruce Schuchardt wrote:
>     There's no reason to have JRE jars in the classpath.

ok, makes sense.


> On Sept. 22, 2016, 9:07 p.m., Dan Smith wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/OldClientSupport.java,
> >  line 33
> > <https://reviews.apache.org/r/52172/diff/1/?file=1508448#file1508448line33>
> >
> >     How is this extensible? This is a static method in a class?
> 
> Bruce Schuchardt wrote:
>     A jar will go on the server's classpath before geode jars.  It will 
> contain an OldClientSupport implementation that will translate the exception 
> to a com.gemstone.gemfire exception for old clients.

Consider using a ServiceProvider instead? It's confusing to have multiple 
copies of a class on the classpath and rely on one overriding the other, and it 
can get tricky when you have automated tools building your classpath (gradle, 
eclipse, intellij, etc.).

Also, it's probably worth thinking about trying to generalize the extension 
point a little bit more. For example, the version check probably belongs in the 
extension, not in the product code. Also consider adding unit tests of the 
extension mechanism in geode.


> On Sept. 22, 2016, 9:07 p.m., Dan Smith wrote:
> > geode-core/src/test/java/org/apache/geode/test/dunit/standalone/ProcessManager.java,
> >  line 188
> > <https://reviews.apache.org/r/52172/diff/1/?file=1508461#file1508461line188>
> >
> >     What is this for?
> 
> Bruce Schuchardt wrote:
>     This will be used for backward-compatibility testing.  It will point to 
> the test output directory so we can find builds of old clients.

Long term, I think we want to separate out the dunit framework from geode so it 
can be used generically for lots of projects. If possible, we should avoid 
having a bunch of special case code for specific geode tests in the famework. 
Rather we should focus on having a generic, extensible framework. Is it 
possible to pass this test specific property to the child VMs as part of the 
test itself?


- Dan


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


On Sept. 22, 2016, 7:31 p.m., Bruce Schuchardt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52172/
> -----------------------------------------------------------
> 
> (Updated Sept. 22, 2016, 7:31 p.m.)
> 
> 
> Review request for geode, Hitesh Khamesra and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-1927
>     https://issues.apache.org/jira/browse/GEODE-1927
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> This adds a check in InternalDataSerializer for com.gemstone.gemfire packages 
> and transforms them to org.apache.geode.
> 
> I've also added a hook for translating org.apache.geode exceptions into 
> com.gemstone.gemfire exceptions.  We've decided to keep com.gemstone.gemfire 
> exceptions out of the Geode repository to avoid the confusion it would cause 
> to have an org.apache.geode implementation throw com.gemstone.gemfire 
> exceptions.
> 
> The latter change required changes to some tests using mocks to represent 
> ServerConnections.  These mocks were returning null from getClientVersion(), 
> causing NPEs in client/server code.
> 
> 
> Diffs
> -----
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java
>  6e7aa55bd8a8b99c63f39965daa0222f42f64d30 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/BaseCommand.java
>  9a78772c466996f83383f4e63cb3d1d6654172a0 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/OldClientSupport.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/ContainsKey66Test.java
>  6728a377816e521a9419e8dd7576397f8548c14e 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/CreateRegionTest.java
>  389399191a3a69a1f591d029ecf3ea3981f845db 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/Destroy65Test.java
>  ed76cb6a965208b315ebb004cef39ea763d4b686 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/DestroyRegionTest.java
>  49a95a0bdae5f4fb51c0d6fcd8d5513805b45d26 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/DestroyTest.java
>  422733e4f5e958add74b4b7586bb123703776bc7 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/Get70Test.java
>  4b63a072370746e950ef3697358c7be7a125a213 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/InvalidateTest.java
>  2dcdd0413c292acd3a9ef40f9809f23f5e95ca29 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/Put61Test.java
>  c368ba89435303f498a212defdc2508426366093 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/Put65Test.java
>  830884c858ae3c6fd4121ed60343e4195b55d77a 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/PutTest.java
>  a9c3af43de1b5b23a958d22b577091e6ccf0bca5 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/RequestTest.java
>  b6997bdef1b51e10780caf1eee6e3a7c46424f98 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/UnregisterInterestTest.java
>  2da6f19a2708b839264c14b9c27e3ff6702a2b52 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/standalone/ProcessManager.java
>  1c87b1a0346445da95c9d510ecc8b8e6e996bac2 
> 
> Diff: https://reviews.apache.org/r/52172/diff/
> 
> 
> Testing
> -------
> 
> precheckin.  New tests are being developed for gemfire<->geode WAN and 
> client/server interactions but these won't be part of the Geode repo.
> 
> 
> Thanks,
> 
> Bruce Schuchardt
> 
>

Reply via email to