[GitHub] [geode] gesterzhou opened pull request #2972: GEODE-6164: CacheClientProxy's closeSocket should be called atomically
Thank you for submitting a contribution to Apache Geode. @jhuynh1 @Bill In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2972 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] gesterzhou opened pull request #2971: NO REVIEW
…taDataOp could end up with NPE (#2952)" Test6149 Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2971 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett opened pull request #2970: DO NOT MERGE: Improves put performance
Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2970 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
Thumbs up! Already approved. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal closed pull request #2950: GEODE-6142: Check JDBC mapping before destroy region
[ pull request closed by dschneider-pivotal ] [ Full content available at: https://github.com/apache/geode/pull/2950 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal opened pull request #2969: GEODE-6156: add --id option to create jdbc-mapping
Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2969 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt opened pull request #2968: GEODE-6143: Removing PowerMock from MBeanProxyFactoryTest
Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [X] Is your initial contribution a single, squashed commit? - [X] Does `gradlew build` run cleanly? - [X] Have you written or updated unit tests to verify your changes? - N/A If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2968 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] WireBaron opened a new pull request #18: GEODE-6163: fix the argument handling in the benchmark run analyzer
WireBaron opened a new pull request #18: GEODE-6163: fix the argument handling in the benchmark run analyzer URL: https://github.com/apache/geode-benchmarks/pull/18 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [geode-benchmarks] WireBaron opened pull request #18: GEODE-6163: fix the argument handling in the benchmark run analyzer
[ Full content available at: https://github.com/apache/geode-benchmarks/pull/18 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
@kirklund No user facing public API packages are being altered in this PR. I think if I had to put my sticking point somewhere it would be on keeping module and artifact names consistent. I am less sticky on the package names inside the modules being consistent with the module name. There is precedent for that even in Java where they have packages `javax.foo` in modules named `java.foo`. So lets do this: Artifact: org.apache.geode:geode-cq (no change from previous releases) Module: org.apache.geode.cq Root package: org.apache.geode.cache.query.cq.internal Yes? [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
The difference between `org.apache.geode.cache.query` and `org.apache.geode.query` is fairly small but they are non-internal User APIs. So for fixing modularization I would lean away from changing the User packages -- or do so as little as possible. The impression I got on the dev-list was that we were just shuffling around internal packages without reshaping our non-internal User packages. You should probably be more explicit about exposing or altering User packages (especially on the dev-list). That's also why I was asking for `org.apache.geode.cache.query.cq` instead of `org.apache.geode.cq` -- because I feel like we're making a pretty permanent decision that impacts the User API/package for CQ. It'll be next to impossible to change that after Geode 1.9.0 releases. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
@kirklund > We can't use org.apache.geode.cache.query.cq inside geode-cq? Or is that just > convention? Since package`org.apache.geode.cache.query.cq` is not in the `geode-core` module we could use that package in `geode-cq` module. It is convention to keep package, module and artifact names similar. There are certainly good reasons to deviate and plenty of examples in the wild where people have. I would be most inclined to keep the module name and artifact name in sync since that is what module automatic naming would do anyway. So artifact `geode-cq.jar` would be automatic module `geode.cq`. So for consistency I would just append the group name, minus redundancy, to get module `org.apache.geode.cq`. In doing that I lean towards keeping the root package name the same, but eh... I do feel like the `cache` part of `o.a.geode.cache.cq` is redundant though. What's he argument for `cache`? > Would we ever create geode-query to contain org.apache.geode.cache.query and > then geode-cq adds cq onto that? Maybe... We can alway refactor later if we do and it feels better to have cq someone logically grouped under query at that point. Having two modules that contain the same bits with different names would be fine as long as one and only one is on the module path at any time. You would simply stop including the deprecated on in the BOM. > I'm thinking that geode-core will eventually contain org.apache.geode.logging > to contain our LogService and SPI for the LogWriter and Alert appenders (or > even move that to geode-logging). Then geode-log4j would contain our default > impls of that SPI which are the LogWriter and Alert appenders that are > written with log4j-core. Then geode-core uses geode-logging and geode-log4j > is an optional provider to the SPI in geode-logging. Or is that too many > sub-modules (I'm new to modules!)? No, that sounds perfect. > So, my input on cq is based mainly on cq being something that builds on > query. If org.apache.geode.cache.query.cq is ugly because of geode-cq or if > we would never split out query to geode-query then my suggestion probably > isn't very relevant. Either way I'll mark my request as resolved and let you > decide which way to go. All valid thoughts. It would be good for us to get it right and not have to deprecate and refactor a module later. I guess I could fall sort of in the middle. I wouldn't have the package name to deviate too far from the module name unless there is a really strong argument. To me the "cache" part feels redundant since Geode is the "cache". So I could see module and package name of `o.a.geode.query.cq` and deviating only in the artifact name not match at this point and remaining `geode-cq`. But in my mind adding the "query" part is redundant to the "cq" part and removing it makes all 3 names consistent. Hmmm... Any additional thoughts before I flip a coin? [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
@kirklund > We can't use org.apache.geode.cache.query.cq inside geode-cq? Or is that just > convention? Since package`org.apache.geode.cache.query.cq` is not in the `geode-core` module we could use that package in `geode-cq` module. It is convention to keep package, module and artifact names similar. There are certainly good reasons to deviate and plenty of examples in the wild where people have. I would be most inclined to keep the module name and artifact name in sync since that is what module automatic naming would do anyway. So artifact `geode-cq.jar` would be automatic module `geode.cq`. So for consistency I would just append the group name, minus redundancy, to get module `org.apache.geode.cq`. In doing that I lean towards keeping the root package name the same, but eh... I do feel like the `cache` part of `o.a.geode.cache.cq` is redundant though. What's he argument for `cache`? > Would we ever create geode-query to contain org.apache.geode.cache.query and > then geode-cq adds cq onto that? Maybe... We can alway refactor later if we do and it feels better to have cq someone logically grouped under query at that point. Having two modules that contain the same bits with different names would be fine as long as one and only one is on the module path at any time. You would simply stop including the deprecated on in the BOM. > I'm thinking that geode-core will eventually contain org.apache.geode.logging > to contain our LogService and SPI for the LogWriter and Alert appenders (or > even move that to geode-logging). Then geode-log4j would contain our default > impls of that SPI which are the LogWriter and Alert appenders that are > written with log4j-core. Then geode-core uses geode-logging and geode-log4j > is an optional provider to the SPI in geode-logging. Or is that too many > sub-modules (I'm new to modules!)? No that sounds perfect. > So, my input on cq is based mainly on cq being something that builds on > query. If org.apache.geode.cache.query.cq is ugly because of geode-cq or if > we would never split out query to geode-query then my suggestion probably > isn't very relevant. Either way I'll mark my request as resolved and let you > decide which way to go. All valid thoughts. It would be good for us to get it right and not have to deprecate and refactor a module later. I guess I could fall sort of in the middle. I wouldn't have the package name to deviate too far from the module name unless there is a really strong argument. To me the "cache" part feels redundant since Geode is the "cache". So I could see module and package name of `o.a.geode.query.cq` and deviating only in the artifact name not match at this point and remaining `geode-cq`. But in my mind adding the "query" part is redundant to the "cq" part and removing it makes all 3 names consistent. Hmmm... Any additional thoughts before I flip a coin? [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #2967: GEODE-5971: Have all offline commands extends OfflineGfshCommand inst…
@aditya87 [ Full content available at: https://github.com/apache/geode/pull/2967 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao opened pull request #2967: GEODE-5971: Have all offline commands extends OfflineGfshCommand inst…
…ead of InternalGfshCommand * eventually InternalGfshCommand should be deleted and replaced with GfshCommand Co-authored-by: Peter Tran Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2967 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] nabarunnag opened pull request #2966: DO NOT REVIEW
* writePdx checks whether the object passed is an internal object as the first step and returns false if that is the case. * This prevents unnecessary allocation of memory and putting strain on the GC machinery. Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2966 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner opened pull request #2965: GEODE-6002: Adding configuration persistence for member specific options
- Changed tests that had been expecting member specific configurations to not be persisted Co-authored-by: Jens Deppe Co-authored-by: Kenneth Howe Co-authored-by: Aditya Anchuri Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [X] Is your initial contribution a single, squashed commit? - [X] Does `gradlew build` run cleanly? - [X] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2965 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] nabarunnag closed pull request #16: GEODE-6084: Refactored the benchmark code as per review.
nabarunnag closed pull request #16: GEODE-6084: Refactored the benchmark code
as per review.
URL: https://github.com/apache/geode-benchmarks/pull/16
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/geode-benchmarks/src/main/java/org/apache/geode/benchmark/parameters/BenchmarkParameters.java
b/geode-benchmarks/src/main/java/org/apache/geode/benchmark/parameters/BenchmarkParameters.java
deleted file mode 100644
index a3c3d3d..000
---
a/geode-benchmarks/src/main/java/org/apache/geode/benchmark/parameters/BenchmarkParameters.java
+++ /dev/null
@@ -1,62 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
- * agreements. See the NOTICE file distributed with this work for additional
information regarding
- * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
- * "License"); you may not use this file except in compliance with the
License. You may obtain a
- * copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
distributed under the License
- * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
- * or implied. See the License for the specific language governing permissions
and limitations under
- * the License.
- */
-package org.apache.geode.benchmark.parameters;
-
-public class BenchmarkParameters {
-
- /**
- * All roles defined for the JVMs created for the benchmark
- */
- public class Roles {
-public static final String SERVER = "server";
-public static final String CLIENT = "client";
-public static final String LOCATOR = "locator";
- }
-
- /**
- * The port used to create the locator for the tests
- */
- public static final int LOCATOR_PORT = 10334;
-
- /**
- * Key range on which all the region operations are conducted on the default
runner
- */
- public static final long KEY_RANGE = 1000;
-
- /**
- * Warm up time for the benchmark running on the default runner
- */
- public static final int WARM_UP_TIME = 60;
-
- /**
- * Total duration for which the benchmark will run on the default runner
- */
- public static final int BENCHMARK_DURATION = 240;
-
- /**
- * String key for the server cache attribute in the TestContext's
attributeTree
- */
- public static final String SERVER_CACHE = "SERVER_CACHE";
-
- /**
- * String key for the client cache attribute in the TestContext's
attributeTree
- */
- public static final String CLIENT_CACHE = "CLIENT_CACHE";
-
- /**
- * Key range on which all the region operations are conducted on a minimal
runner
- */
- public static final long KEY_RANGE_FOR_MINIMAL_RUNNER = 5;
-}
diff --git
a/geode-benchmarks/src/main/java/org/apache/geode/benchmark/tasks/CreateClientProxyRegion.java
b/geode-benchmarks/src/main/java/org/apache/geode/benchmark/tasks/CreateClientProxyRegion.java
index ed724ee..5cf673c 100644
---
a/geode-benchmarks/src/main/java/org/apache/geode/benchmark/tasks/CreateClientProxyRegion.java
+++
b/geode-benchmarks/src/main/java/org/apache/geode/benchmark/tasks/CreateClientProxyRegion.java
@@ -14,8 +14,6 @@
*/
package org.apache.geode.benchmark.tasks;
-import static
org.apache.geode.benchmark.parameters.BenchmarkParameters.CLIENT_CACHE;
-
import org.apache.geode.cache.client.ClientCache;
import org.apache.geode.cache.client.ClientRegionShortcut;
import org.apache.geode.perftest.Task;
@@ -27,7 +25,7 @@
public class CreateClientProxyRegion implements Task {
@Override
public void run(TestContext context) throws Exception {
-ClientCache clientCache = (ClientCache) context.getAttribute(CLIENT_CACHE);
+ClientCache clientCache = (ClientCache)
context.getAttribute("CLIENT_CACHE");
clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY)
.create("region");
diff --git
a/geode-benchmarks/src/main/java/org/apache/geode/benchmark/tasks/CreatePartitionedRegion.java
b/geode-benchmarks/src/main/java/org/apache/geode/benchmark/tasks/CreatePartitionedRegion.java
index 546d030..1604dc5 100644
---
a/geode-benchmarks/src/main/java/org/apache/geode/benchmark/tasks/CreatePartitionedRegion.java
+++
b/geode-benchmarks/src/main/java/org/apache/geode/benchmark/tasks/CreatePartitionedRegion.java
@@ -15,8 +15,6 @@
package org.apache.geode.benchmark.tasks;
-import static
org.apache.geode.benchmark.parameters.BenchmarkParameters.SERVER_CACHE;
-
import org.apache.geode.cache.Cache;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.perftest.Task;
@@ -29,7 +27,7 @@
@Override
public void run(TestContext context) throws Exception {
-Cache cache = (Cache)
[GitHub] [geode-benchmarks] nabarunnag closed pull request #16: GEODE-6084: Refactored the benchmark code as per review.
[ pull request closed by nabarunnag ] [ Full content available at: https://github.com/apache/geode-benchmarks/pull/16 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] balesh2 opened pull request #2964: Geode 6112
Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2964 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied opened pull request #2963: [Do not review] Exploratory testing of dependencies.
Hello all. I wanted to see if the dependencies that Nebula moved / changed the scope / removed breaks under precheckin. No need to review, as this should be split into multiple PRs, and the POM changes might warrant discussion. [ Full content available at: https://github.com/apache/geode/pull/2963 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao closed pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
[ pull request closed by jinmeiliao ] [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
I forgot geode-cq is a module but geode-query is not [edited]. It still seems like cq is part of query(ing) though. We can't use org.apache.geode.cache.query.cq inside geode-cq? Or is that just convention? Would we ever create geode-query to contain org.apache.geode.cache.query and then geode-cq adds cq onto that? I'm thinking that geode-core will eventually contain org.apache.geode.logging to contain our LogService and SPI for the LogWriter and Alert appenders (or even move that to geode-logging). Then geode-log4j would contain our default impls of that SPI which are the LogWriter and Alert appenders that are written with log4j-core. Then geode-core uses geode-logging and geode-log4j is an optional provider to the SPI in geode-logging. Or is that too many sub-modules (I'm new to modules!)? So, my input on cq is based mainly on cq being something that builds on query. If org.apache.geode.cache.query.cq is ugly because of geode-cq or if we would never split out query to geode-query then my suggestion probably isn't very relevant. Either way I'll mark my request as resolved and let you decide which way to go. [I edited the package names to include ".cache."] [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
I forgot geode-cq is a module but geode-query is not [edited]. It still seems like cq is part of query(ing) though. We can't use org.apache.geode.query.cq inside geode-cq? Or is that just convention? Would we ever create geode-query to contain org.apache.geode.query and then geode-cq adds cq onto that? I'm thinking that geode-core will eventually contain org.apache.geode.logging to contain our LogService and SPI for the LogWriter and Alert appenders (or even move that to geode-logging). Then geode-log4j would contain our default impls of that SPI which are the LogWriter and Alert appenders that are written with log4j-core. Then geode-core uses geode-logging and geode-log4j is an optional provider to the SPI in geode-logging. Or is that too many sub-modules (I'm new to modules!)? So, my input on cq is based mainly on cq being something that builds on query. If org.apache.geode.query.cq is ugly because of geode-cq or if we would never split out query to geode-query then my suggestion probably isn't very relevant. Either way I'll mark my request as resolved and let you decide which way to go. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
I forgot about geode-cq as a module. It still seems like cq is part of query(ing) though. We can't use org.apache.geode.query.cq inside geode-cq? Or is that just convention? Would we ever create geode-query to contain org.apache.geode.query and then geode-cq adds cq onto that? I'm thinking that geode-core will eventually contain org.apache.geode.logging to contain our LogService and SPI for the LogWriter and Alert appenders (or even move that to geode-logging). Then geode-log4j would contain our default impls of that SPI which are the LogWriter and Alert appenders that are written with log4j-core. Then geode-core uses geode-logging and geode-log4j is an optional provider to the SPI in geode-logging. Or is that too many sub-modules (I'm new to modules!)? So, my input on cq is based mainly on cq being something that builds on query. If org.apache.geode.query.cq is ugly because of geode-cq or if we would never split out query to geode-query then my suggestion probably isn't very relevant. Either way I'll mark my request as resolved and let you decide which way to go. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Okay. Just wondering. I see that the regular "GfshCommand" class also has access to the current Gfsh instance, though. So I'm sure there's something here which I'm not understanding, we can discuss that another time :). [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Oh, that statement should have a clause: with the same test coverage, junit is preferred than integration test, integration test is preferred than dunit. Like you don't need a dunit test to test a invalid command option. [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Oh, that statement should have a clause: with the same test coverage, junit is preferred than integration test, integration test is preferred than dunit [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
I am convinced that we don't need a DUnit test for this one. However, I'm not sure I agree with the general principle of not preferring integration tests? Especially in a distributed system like Geode, I feel that the greater robustness and less mocky nature of integration tests trumps the ease of controlling more mock-heavy "unit" tests. Ideally I'd want both, i.e. integration tests and unit tests. We can discuss this point outside of this PR. [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
I am convinced that we don't need a DUnit test for this one. However, I'm not sure I agree with the general principle of not having integration tests? Especially in a distributed system like Geode, I feel that the greater robustness and less mocky nature of integration tests trumps the ease of controlling more mock-heavy "unit" tests. Ideally I'd want both, i.e. integration tests and unit tests. We can discuss this point outside of this PR. [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Okay. Just wondering. [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt closed pull request #2959: GEODE-6143: Remove PowerMock from CacheClientNotifierIntegrationTest
[ pull request closed by mcmellawatt ] [ Full content available at: https://github.com/apache/geode/pull/2959 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] gesterzhou closed pull request #2952: GEODE-6149: when client's cache is closing, its GetClientPRMetaDataOp…
[ pull request closed by gesterzhou ] [ Full content available at: https://github.com/apache/geode/pull/2952 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
These commands are different from other commands that it doesn't go through that usual workflow. This is simply gather the options and start a jConsole process. Our future rest service would have nothing to do with it. It needs InternalGfshCommand since it needs reference to the gfsh instance to get the invoker. [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
for the changes we are making, a junit test should do. An end to end test would need to fire up additional process, hard to control in a test environment. We should always prefer junit test over integration test and integration over dunit tests. Use dunit tests only when necessary. [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] upthewaterspout commented on issue #2961: GEODE-6154: Remove optional from geode-core spring-shell dependency
I put a comment on your JIRA - it seems that ClientCacheFactory actually doesn't require spring shell - just the way you are creating a client, thanks to some crazy if checks. That said, this is still a mess. Seems like we should remove all of the optional flags, maybe? Or actually refactor these management related dependencies out of geode-core! [ Full content available at: https://github.com/apache/geode/pull/2961 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Shouldn't this be SingleGfshCommand or GfshCommand? [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Curious, do we not have more end-to-end test coverage for this command? [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2960: GEODE-5971: Refactor StatusClusterConfigServiceCommand to extend GfshCommand base type
no need to do this. all locators/servers started by the rule will be shut down by the rule. [ Full content available at: https://github.com/apache/geode/pull/2960 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-benchmarks] upthewaterspout commented on pull request #16: GEODE-6084: Refactored the benchmark code as per review.
Camel case package name needs a bit weird. Does the JVMParameters class even need to be a it's own package? seems like it shouldn't be under client/server [ Full content available at: https://github.com/apache/geode-benchmarks/pull/16 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] upthewaterspout commented on a change in pull request #16: GEODE-6084: Refactored the benchmark code as per review.
upthewaterspout commented on a change in pull request #16: GEODE-6084:
Refactored the benchmark code as per review.
URL: https://github.com/apache/geode-benchmarks/pull/16#discussion_r239613439
##
File path:
geode-benchmarks/src/main/java/org/apache/geode/benchmark/tests/ClientServerTopology/parameters/JVMParamters.java
##
@@ -12,10 +12,9 @@
* or implied. See the License for the specific language governing permissions
and limitations under
* the License.
*/
-package org.apache.geode.benchmark.parameters;
-
-public class JVMParameters {
+package org.apache.geode.benchmark.tests.ClientServerTopology.parameters;
Review comment:
Camel case package name needs a bit weird. Does the JVMParameters class even
need to be a it's own package? seems like it shouldn't be under client/server
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [geode-benchmarks] upthewaterspout commented on pull request #16: GEODE-6084: Refactored the benchmark code as per review.
Typo - Paramters instead of Parameters. [ Full content available at: https://github.com/apache/geode-benchmarks/pull/16 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] upthewaterspout commented on a change in pull request #16: GEODE-6084: Refactored the benchmark code as per review.
upthewaterspout commented on a change in pull request #16: GEODE-6084:
Refactored the benchmark code as per review.
URL: https://github.com/apache/geode-benchmarks/pull/16#discussion_r239613550
##
File path:
geode-benchmarks/src/main/java/org/apache/geode/benchmark/tests/ClientServerTopology/parameters/JVMParamters.java
##
@@ -12,10 +12,9 @@
* or implied. See the License for the specific language governing permissions
and limitations under
* the License.
*/
-package org.apache.geode.benchmark.parameters;
-
-public class JVMParameters {
+package org.apache.geode.benchmark.tests.ClientServerTopology.parameters;
+public class JVMParamters {
Review comment:
Typo - Paramters instead of Parameters.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [geode-native] pdxcodemonkey opened pull request #416: GEODE-6160: Properly escape backslashes in .cpackignore
Co-authored-by: Matthew Reddington [ Full content available at: https://github.com/apache/geode-native/pull/416 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] bschuchardt commented on issue #2956: GEODE-2113 Implement SSL over NIO
dunit tests timed out - I'm checking into it [ Full content available at: https://github.com/apache/geode/pull/2956 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] BenjaminPerryRoss commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
The reason we elected to use a new class scoped data source class was so that in the future if which data sources are valid changes this test will still be comparing against an 'invalid' data source class. We originally tried to compare it against a Managed or XAPooled data source class but ran into some trouble with that and this solution seemed more accurate. [ Full content available at: https://github.com/apache/geode/pull/2918 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao closed pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand
[ pull request closed by jinmeiliao ] [ Full content available at: https://github.com/apache/geode/pull/2875 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao opened pull request #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Co-authored-by: Peter Tran Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
Ah, makes sense and good to hear. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #2962: GEODE-5971: refactor StartJConsoleCommand to use ResultModel
@aditya87 [ Full content available at: https://github.com/apache/geode/pull/2962 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett closed pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map
[ pull request closed by pivotal-jbarrett ] [ Full content available at: https://github.com/apache/geode-native/pull/401 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
Should also mention that it goes away completely with the upcoming `module-info.java` changes. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
The modules "plugin" expects it project. It is consistent with the experimental plugin from Gradle. Eventually they will have first level support for this. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund opened pull request #2961: GEODE-6154: Remove optional from geode-core spring-shell dependency
[ Full content available at: https://github.com/apache/geode/pull/2961 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
[ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
These are the minimum changes to get modules working from a client. Yes other deps may be old but out of scope for this work. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
Always love to see these simple sorts of cleanups. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
Just a reminder that, by popular request, `spotless` will no longer remove extra whitespace within a line, even if the result will no longer exceed the 80-width limit. For instance, this line's diff isn't necessary for spotless (but I assume it was at some point during your iteration). [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
I'm really not a fan of bloating out the `ext` in our project / subprojects. Is there a reason this shouldn't just be in-lined below? [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
I don't really know what this file is documenting, but every other version here is wildly out of date [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal closed pull request #2957: GEODE-6151: use same term for JDBC mapping
[ pull request closed by dschneider-pivotal ] [ Full content available at: https://github.com/apache/geode/pull/2957 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] sboorlagadda opened pull request #415: GEODE-5957: Parse (previously) unknown server error messages
- Handle request data error messages for function execution on a region. Signed-off-by: Ernest Burghardt [ Full content available at: https://github.com/apache/geode-native/pull/415 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-benchmarks] nabarunnag closed pull request #17: Fixing the warnings mentioned by LGTM.
[ pull request closed by nabarunnag ] [ Full content available at: https://github.com/apache/geode-benchmarks/pull/17 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] nabarunnag closed pull request #17: Fixing the warnings mentioned by LGTM.
nabarunnag closed pull request #17: Fixing the warnings mentioned by LGTM.
URL: https://github.com/apache/geode-benchmarks/pull/17
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/harness/src/main/java/org/apache/geode/perftest/jvms/classpath/JarUtil.java
b/harness/src/main/java/org/apache/geode/perftest/jvms/classpath/JarUtil.java
index a8e6adc..eac6ccb 100644
---
a/harness/src/main/java/org/apache/geode/perftest/jvms/classpath/JarUtil.java
+++
b/harness/src/main/java/org/apache/geode/perftest/jvms/classpath/JarUtil.java
@@ -42,8 +42,8 @@
*/
static void jar(File file, File outputFile) throws IOException {
Manifest manifest = new Manifest();
-try (JarOutputStream outputStream =
-new JarOutputStream(new FileOutputStream(outputFile), manifest)) {
+try (FileOutputStream fileOutputStream = new FileOutputStream(outputFile);
+JarOutputStream outputStream = new JarOutputStream(fileOutputStream,
manifest)) {
Path start = file.toPath();
Files.walkFileTree(start, new SimpleFileVisitor() {
diff --git
a/harness/src/main/java/org/apache/geode/perftest/jvms/rmi/ChildJVM.java
b/harness/src/main/java/org/apache/geode/perftest/jvms/rmi/ChildJVM.java
index 3a25b15..5603cb0 100644
--- a/harness/src/main/java/org/apache/geode/perftest/jvms/rmi/ChildJVM.java
+++ b/harness/src/main/java/org/apache/geode/perftest/jvms/rmi/ChildJVM.java
@@ -65,32 +65,33 @@ void run() {
// Clean up the output directory before the test runs
FileUtils.deleteQuietly(outputDir);
outputDir.mkdirs();
- PrintStream out = new PrintStream(new File(outputDir, "system.log"));
- system.setOut(out);
- system.setErr(out);
+ try (PrintStream out = new PrintStream(new File(outputDir,
"system.log"))) {
+system.setOut(out);
+system.setErr(out);
- ControllerRemote controller = (ControllerRemote) rmi
- .lookup("//" + RMI_HOST + ":" + RMI_PORT + "/" +
RemoteJVMFactory.CONTROLLER);
+ControllerRemote controller = (ControllerRemote) rmi
+.lookup("//" + RMI_HOST + ":" + RMI_PORT + "/" +
RemoteJVMFactory.CONTROLLER);
- SharedContext sharedContext = controller.getsharedContext();
- DefaultTestContext context = new DefaultTestContext(sharedContext,
outputDir, id);
+SharedContext sharedContext = controller.getsharedContext();
+DefaultTestContext context = new DefaultTestContext(sharedContext,
outputDir, id);
- Worker worker = new Worker(context);
+Worker worker = new Worker(context);
- controller.addWorker(id, worker);
+controller.addWorker(id, worker);
- // Wait until the controller shuts down
- // If the controller shuts down, this will throw an exception
- try {
-while (controller.ping()) {
- Thread.sleep(pingTime);
+// Wait until the controller shuts down
+// If the controller shuts down, this will throw an exception
+try {
+ while (controller.ping()) {
+Thread.sleep(pingTime);
+ }
+} catch (RemoteException e) {
+ // If we get a RemoteException, the controller has shut down
+ // exit gracefully
}
- } catch (RemoteException e) {
-// If we get a RemoteException, the controller has shut down
-// exit gracefully
- }
- system.exit(0);
+system.exit(0);
+ }
} catch (Throwable t) {
t.printStackTrace();
// Force a system exit. Because we created an RMI object, an exception
from the main
diff --git
a/harness/src/main/java/org/apache/geode/perftest/yardstick/analysis/YardstickPercentileSensorParser.java
b/harness/src/main/java/org/apache/geode/perftest/yardstick/analysis/YardstickPercentileSensorParser.java
index d99a538..7db3fec 100644
---
a/harness/src/main/java/org/apache/geode/perftest/yardstick/analysis/YardstickPercentileSensorParser.java
+++
b/harness/src/main/java/org/apache/geode/perftest/yardstick/analysis/YardstickPercentileSensorParser.java
@@ -57,16 +57,18 @@
public void parseResults(File resultDir) throws IOException {
File sensorData = new File(resultDir, sensorOutputFile);
-BufferedReader dataStream = new BufferedReader(new FileReader(sensorData));
-String nextLine;
-
-while ((nextLine = dataStream.readLine()) != null) {
- if (nextLine.startsWith("--") ||
- nextLine.startsWith("@@") ||
- nextLine.startsWith("**")) {
-continue;
+try (FileReader fileReader = new FileReader(sensorData);
+BufferedReader dataStream = new BufferedReader(fileReader)) {
+ String nextLine;
+
+ while ((nextLine = dataStream.readLine()) != null) {
+if
[GitHub] [geode] aditya87 commented on issue #2960: GEODE-5971: Refactor StatusClusterConfigServiceCommand to extend GfshCommand base type
@jinmeiliao Please take a look and review! [ Full content available at: https://github.com/apache/geode/pull/2960 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 opened pull request #2960: GEODE-5971: Refactor StatusClusterConfigServiceCommand to extend GfshCommand base type
- Added missing DUnit test for the command. Signed-off-by: Ken Howe Signed-off-by: Aditya Anchuri Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [X] Is your initial contribution a single, squashed commit? - [X] Does `gradlew build` run cleanly? - [X] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2960 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
@kirklund I tried to keep the package names, module names and project names consistent. I am up for any naming really but we need consistency that we don't have today. The rational for package `org.apache.geode.cq` is that fits with the Maven coordinate of `org.apache.geode:geode-cq` and the module name of `org.apache.geode.cq`. So you are asking for classes in `org.apache.geode.cache.query.internal.cq` to move to `org.apache.geode.cache.query.cq.internal`? I think over time what we will see then is that stuff in `gemfire-core` at `org.apache.geode.cache.query.internal.cq` should migrate to `org.apache.geode.cache.query.cq.spi` and things in `geode-cq` that implement those SPIs will move to `org.apache.geode.cache.query.cq.impl` or something like that. Feels a little verbose but works for me I guess. What module name would you suggest? [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner commented on issue #2926: GEODE-5971: Refactor ShowDeadlockCommand to return ResultModel and ex…
Closed. This PR has been superseded by PR#2943 [ Full content available at: https://github.com/apache/geode/pull/2926 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner closed pull request #2926: GEODE-5971: Refactor ShowDeadlockCommand to return ResultModel and ex…
[ pull request closed by pdxrunner ] [ Full content available at: https://github.com/apache/geode/pull/2926 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
@kirklund I tried to keep the package names, module names and project names consistent. I am up for any naming really but we need consistency that we don't have today. So you are asking for classes in `org.apache.geode.cache.query.internal.cq` to move to `org.apache.geode.cache.query.cq.internal`? I think over time what we will see then is that stuff in `gemfire-core` at `org.apache.geode.cache.query.internal.cq` should migrate to `org.apache.geode.cache.query.cq.spi` and things in `geode-cq` that implement those SPIs will move to `org.apache.geode.cache.query.cq.impl` or something like that. Feels a little verbose but works for me I guess. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt opened pull request #2959: GEODE-6143: Remove PowerMock from CacheClientNotifierIntegrationTest
Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [X] Is your initial contribution a single, squashed commit? - [X] Does `gradlew build` run cleanly? - [X] Have you written or updated unit tests to verify your changes? - N/A If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/2959 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner closed pull request #2943: GEODE-5971: Refactor ShowDeadlockCommand to return ResultModel and ex…
[ pull request closed by pdxrunner ] [ Full content available at: https://github.com/apache/geode/pull/2943 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied closed pull request #2940: GEODE-6129: Make dependencies explicit in geode-wan
[ pull request closed by PurelyApplied ] [ Full content available at: https://github.com/apache/geode/pull/2940 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand
This is where the AssemblyContentIntegrationTest dumps the file it generates (at least on my workstation). I copy this file to the src folder that it is supposed to reference, but this file itself I usually delete so that it doesn't get checked in. I thought I'd gitignore it so I don't have to keep deleting it. [ Full content available at: https://github.com/apache/geode/pull/2875 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand
Why is this change needed? [ Full content available at: https://github.com/apache/geode/pull/2875 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund closed pull request #2944: GEODE-6122: Make log4j core optional
[ pull request closed by kirklund ] [ Full content available at: https://github.com/apache/geode/pull/2944 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2938: GEODE-3613: Allocate unique ports to containers
The log4j syntax uses parameterization:
```
logger.info("Starting container {} RMI Port: {}", description, jvmJmxPort);
```
[ Full content available at: https://github.com/apache/geode/pull/2938 ]
This message was relayed via gitbox.apache.org for
notifications@geode.apache.org
[GitHub] [geode] pdxrunner commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand
+1 [ Full content available at: https://github.com/apache/geode/pull/2875 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt closed pull request #2955: GEODE-6143: Removing PowerMock from BackupFileCopierIntegrationTest
[ pull request closed by mcmellawatt ] [ Full content available at: https://github.com/apache/geode/pull/2955 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand
Can we mark this with a JIRA ticket number, so we can track it? [ Full content available at: https://github.com/apache/geode/pull/2875 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] nabarunnag opened a new pull request #17: Fixing the warnings mentioned by LGTM.
nabarunnag opened a new pull request #17: Fixing the warnings mentioned by LGTM. URL: https://github.com/apache/geode-benchmarks/pull/17 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
I know right? This is a result of us putting `java` in a `subproject` block. Gradle treats all directories as projects so all directories are Java! [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2950: GEODE-6142: Check JDBC mapping before destroy region
will do [ Full content available at: https://github.com/apache/geode/pull/2950 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2950: GEODE-6142: Check JDBC mapping before destroy region
will do [ Full content available at: https://github.com/apache/geode/pull/2950 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2950: GEODE-6142: Check JDBC mapping before destroy region
apis do not change cluster config. Other than that api region destroy does what it always has done. The only additional thing you would want to remove when using the apis is the async queue on the cache. Of course this is already true for the apis; if the region is the last one using the queue then destroy region does not remove that queue. [ Full content available at: https://github.com/apache/geode/pull/2950 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] metatype commented on pull request #2915: GEODE-6117: Makes modules out of geode-core and geode-cq
No changes needed, but it would be nice if the build didn't assume that every subdirectory is a `java` project. [ Full content available at: https://github.com/apache/geode/pull/2915 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #2950: GEODE-6142: Check JDBC mapping before destroy region
My concern is that this would make the region commands aware of a specific extension point, which makes it not extensible after all. JDBC is an extension of the core, I guess the idea is some customer might install geode without installing this extension. Is it right for the core to depend on jdbc code? [ Full content available at: https://github.com/apache/geode/pull/2950 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2950: GEODE-6142: Check JDBC mapping before destroy region
same here, you can use ```suggestion RegionMapping mapping = CacheElement.findElement(cacheConfig.getCustomRegionElements(), "jdbc-mapping") ``` [ Full content available at: https://github.com/apache/geode/pull/2950 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2950: GEODE-6142: Check JDBC mapping before destroy region
I believe you can use ```suggestion RegionConfig regionConfig = CacheElement.findElement(cacheConfig.getRegions, regionName) ``` [ Full content available at: https://github.com/apache/geode/pull/2950 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
