[GitHub] [geode] jinmeiliao commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
Nice, but if it's only used by the RegionConfigFactory, they don't belong in this package either. Make it something private to the command. [ 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 commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
Generally, I would like to see us moving away from using RegionFunctionArgs. In the command, we should start creating the RegionConfig object directly from the command arguments. This RegionFunctionArgs is there before the config objects were not available before. If we can completely get rid of

[GitHub] WireBaron opened a new pull request #1: Adding new class to evaluate a test run against a baseline and output…

2018-11-19 Thread GitBox
WireBaron opened a new pull request #1: Adding new class to evaluate a test run against a baseline and output… URL: https://github.com/apache/geode-benchmarks/pull/1 … the relative performance. This is an automated

[GitHub] [geode-benchmarks] WireBaron opened pull request #1: Adding new class to evaluate a test run against a baseline and output…

2018-11-19 Thread GitHub
… the relative performance. [ Full content available at: https://github.com/apache/geode-benchmarks/pull/1 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #406: GEODE-4728: Removed all usage of grid client

2018-11-19 Thread GitHub
This may be a redundant test if it was initializing grid client. Is there another test that covers these same methods but didn't init grid? [ Full content available at: https://github.com/apache/geode-native/pull/406 ] This message was relayed via gitbox.apache.org for notifications@geode.apache

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #406: GEODE-4728: Removed all usage of grid client

2018-11-19 Thread GitHub
Duplicate test? [ Full content available at: https://github.com/apache/geode-native/pull/406 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #406: GEODE-4728: Removed all usage of grid client

2018-11-19 Thread GitHub
Does removing this create a duplicate test where the same methods are being exercised already without grid set? [ Full content available at: https://github.com/apache/geode-native/pull/406 ] 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

2018-11-19 Thread GitHub
Yes, this and the RegionAttributeSetFunction are "functional interfaces" which we are using as lambda functions to create and retrieve different kinds of region attributes. Please see RegionConfigFactory for an example of how this is used. If you haven't heard of it before, it's actually a pre

[GitHub] [geode] aditya87 commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
See comment above [ 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] aditya87 commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
Sure -- we can do that instead of bounce. [ 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] aditya87 commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
I agree that the RegionConfigFactory should be in the command directory -- we will move that. With regards to the RegionFunctionArgs -- yes, we should ideally not use it, but we wanted to do this in the most incremental fashion possible. The most important takeaway here is moving away from gener

[GitHub] [geode] kirklund commented on pull request #2866: GEODE-5076 jmx client should not access or modify internal regions

2018-11-19 Thread GitHub
You could probably revert the above line and just keep the if-block you added because it fetches getCacheForProcessingClientRequests. [ Full content available at: https://github.com/apache/geode/pull/2866 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #2866: GEODE-5076 jmx client should not access or modify internal regions

2018-11-19 Thread GitHub
CacheServerBridge already has a reference to the cache, so you could replace the singleton caller to: ``` InternalCache cache = cache.getCacheForProcessingClientRequests(); ``` [ Full content available at: https://github.com/apache/geode/pull/2866 ] This message was relayed via gitbox.apache.org

[GitHub] [geode] kirklund commented on pull request #2866: GEODE-5076 jmx client should not access or modify internal regions

2018-11-19 Thread GitHub
PS: I think I'm mixing up getCacheForProcessingClientRequests and getCacheForProcessingClientAccess. Do we really need both? [ Full content available at: https://github.com/apache/geode/pull/2866 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #2866: GEODE-5076 jmx client should not access or modify internal regions

2018-11-19 Thread GitHub
I think this should probably change to: ``` InternalCache cache = toInternalCacheForClientAccess(context.getCache()); ``` Then you could replace ManagementAgent.getCache() with: ``` public static InternalCacheForClientAccess toInternalCacheForClientAccess(Cache cache) { return ((InternalCache)c

[GitHub] [geode-native] echobravopapa opened pull request #406: GEODE-4728: Removed all usage of grid client

2018-11-19 Thread GitHub
[ Full content available at: https://github.com/apache/geode-native/pull/406 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund opened pull request #2879: GEODE-1585: Cleanup SystemAdminDUnitTest

2018-11-19 Thread GitHub
Use Rules and AssertJ so that actionable info is available the next time this test fails. [ Full content available at: https://github.com/apache/geode/pull/2879 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
Do we need this? [ 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 commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
Generally, I would like to see us moving away from using RegionFunctionArgs. In the command, we should start creating the RegionConfig object directly from the command arguments. This RegionFunctionArgs is there before the config objects are available. If we can completely get rid of it, that wo

[GitHub] [geode] jinmeiliao commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
bounce actually restart the java process. you can do server1.stop() which shutdown the server and clean out the working dir as well. [ 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 commented on pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
what is this for? [ 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 opened pull request #2878: GEODE-5547: Log info message when canceling ManagementListener

2018-11-19 Thread GitHub
This adds a benevolent info level log message to help diagnose GEODE-5547 the next time it fails in CI. I added some comments to GEODE-5547 with my analysis of the latest reported failure. [ Full content available at: https://github.com/apache/geode/pull/2878 ] This message was relayed via gitb

[GitHub] [geode] rhoughton-pivot closed pull request #2832: GEODE-6026: Fixes inclusion of test classes in JavaDocs.

2018-11-19 Thread GitHub
[ pull request closed by rhoughton-pivot ] [ Full content available at: https://github.com/apache/geode/pull/2832 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #2832: GEODE-6026: Fixes inclusion of test classes in JavaDocs.

2018-11-19 Thread GitHub
Yes, this was invoking the javadocs on the modules and then here as well. We don't actually bundle the javadocs from the modules. We build them here from the sources in each of the modules. [ Full content available at: https://github.com/apache/geode/pull/2832 ] This message was relayed via git

[GitHub] [geode] rhoughton-pivot opened pull request #2877: GEODE-6059 - PR pipeline gets merge-base from resource

2018-11-19 Thread GitHub
Keeps the PR from needing the entire repo for faster testing Authored-by: Robert Houghton 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

[GitHub] [geode] metatype closed pull request #2868: GEODE-6063 remove PublishArtifacts from Geode release pipelines (#2865)

2018-11-19 Thread GitHub
[ pull request closed by metatype ] [ Full content available at: https://github.com/apache/geode/pull/2868 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] rhoughton-pivot opened pull request #2876: GEODE-6046 - Java memory allocation issue JDK-8207200

2018-11-19 Thread GitHub
Java MemoryUsage object has a new exception in Java9+ that is not fixed until Java 12. Add an ignore to the log checker to ignore the suspect string. Authored-by: Robert Houghton Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we as

[GitHub] [geode] aditya87 opened pull request #2875: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
- Store config via generating a RegionConfig object rather than an XML entity - Store config via generating a RegionConfig object rather than an XML entity - For clarity, rename getters that return a "Boolean" object rather than a boolean primitive as prefixed by "get" rather than "is". This commu

[GitHub] [geode] aditya87 commented on issue #2874: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
Withdrawing, will recreate once we fix some issues [ Full content available at: https://github.com/apache/geode/pull/2874 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 closed pull request #2874: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
[ pull request closed by aditya87 ] [ Full content available at: https://github.com/apache/geode/pull/2874 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #2874: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
Sorry -- this might have been added when I temporarily brought in lombok, I realize it's unnecessary. Will remove. [ Full content available at: https://github.com/apache/geode/pull/2874 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jdeppe-pivotal closed pull request #2854: GEODE-5971: Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand

2018-11-19 Thread GitHub
[ pull request closed by jdeppe-pivotal ] [ Full content available at: https://github.com/apache/geode/pull/2854 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] rhoughton-pivot commented on pull request #2874: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
Why was this needed? `mavenCentral` is already a repository for `allprojects`, added in the project root `build.gradle` [ Full content available at: https://github.com/apache/geode/pull/2874 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #2854: GEODE-5971: Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand

2018-11-19 Thread GitHub
For better readability: if(!StringUtils.isBlank){ // use the group } else if (command is instance of UpdateAllMaker){ // pass all groups } else{ // pass in "cluster" } [ Full content available at: https://github.com/apache/geode/pull/2854 ] This message was relayed via gitbox.apache.org for notif

[GitHub] [geode] mcmellawatt closed pull request #2871: GEODE-6072: Clean up thread local between tests

2018-11-19 Thread GitHub
[ pull request closed by mcmellawatt ] [ Full content available at: https://github.com/apache/geode/pull/2871 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 opened pull request #2874: GEODE-5971: Refactor CreateRegionCommand to extend SingleGfshCommand

2018-11-19 Thread GitHub
- Store config via generating a RegionConfig object rather than an XML entity - For clarity, rename getters that return a "Boolean" object rather than a boolean primitive as prefixed by "get" rather than "is". This communicates nullability to the caller. A bit of history here -- this code was ori

[GitHub] [geode] mcmellawatt commented on pull request #2863: GEODE-6064: redact the password in describeConfig command

2018-11-19 Thread GitHub
Why were we able to get rid of this exception handling and the catch-all below? Does the caller do the handling now? [ Full content available at: https://github.com/apache/geode/pull/2863 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt closed pull request #2810: checkRegisteredKeys function consumes alot of CPU - GEODE-5887

2018-11-19 Thread GitHub
[ pull request closed by bschuchardt ] [ Full content available at: https://github.com/apache/geode/pull/2810 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org