You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2019/12/23 14:16:18 UTC
[GitHub] [calcite] XuQianJin-Stars opened a new pull request #1683:
[CALCITE-3622] Update geode tests upgrade from junit4 to junit5
XuQianJin-Stars opened a new pull request #1683: [CALCITE-3622] Update geode tests upgrade from junit4 to junit5
URL: https://github.com/apache/calcite/pull/1683
As illustrated in CALCITE-3622
Update `geode` tests upgrade from junit4 to junit5.
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi closed pull request #1683: [CALCITE-3622] Update
geode tests upgrade from junit4 to junit5
Posted by GitBox <gi...@apache.org>.
vlsi closed pull request #1683: [CALCITE-3622] Update geode tests upgrade from junit4 to junit5
URL: https://github.com/apache/calcite/pull/1683
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #1683:
[CALCITE-3622] Update geode tests upgrade from junit4 to junit5
Posted by 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