[GitHub] [calcite] vlsi commented on a change in pull request #1683: [CALCITE-3622] Update geode tests upgrade from junit4 to junit5

[GitBox <gi...@apache.org>]

vlsi commented on a change in pull request #1683: [CALCITE-3622] Update geode tests upgrade from junit4 to junit5
URL: https://github.com/apache/calcite/pull/1683#discussion_r361396024
 
 

 ##########
 File path: geode/src/test/java/org/apache/calcite/adapter/geode/rel/GeodeEmbeddedPolicy.java
 ##########
 @@ -126,23 +136,23 @@ static GeodeEmbeddedPolicy create() {
       this.refCount = new AtomicInteger();
     }
 
-    @Override GeodeEmbeddedPolicy share() {
 
 Review comment:
   @XuQianJin-Stars , please refrain from moving unrelated code in the future. You have moved `share()` to the end of the class, and it did make it harder to understand the diff. It is lot a huge issue, but it is something that makes a difference between "ok, I see this PR is good, I will merge it immediately" and "well, I need to load it to my IDE and inspect carefully".
   
   Please compare with https://github.com/apache/calcite/commit/484b112641a1a9780bafc43cda6079ebc4806ca3#diff-6820bea7216536ddc41a040817f66218R135-R145 where the diff much simpler
   
   Note: you did remove `synchronized` modifier from `before()` method, and it is very hard to spot. The modifier was important.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services