[GitHub] [geode] nabarunnag opened pull request #4070: GEODE-7218: Redundant addAll call.

2019-09-19 Thread GitHub
* Using the constructor with initial value rather than using addAll after the constructor. 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: - [ ]

[GitHub] [geode] nabarunnag closed pull request #4069: GEODE-6094: Removing unnecessary String.format.

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

[GitHub] [geode] jujoramos commented on pull request #4061: Geode 6874: add unit tests to tomcat modules

2019-09-19 Thread GitHub
Can we change this to `Cache` and `LogWriter` instead?. [ Full content available at: https://github.com/apache/geode/pull/4061 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4061: Geode 6874: add unit tests to tomcat modules

2019-09-19 Thread GitHub
Do we really need all these new methods to create, write, and other operations on low level java classes just for mocking purposes?. Looks "a bit overkilling" to me... [ Full content available at: https://github.com/apache/geode/pull/4061 ] This message was relayed via gitbox.apache.org for

[GitHub] [geode] jujoramos closed pull request #4030: GEODE-6984: Make OQL Security API Public

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

[GitHub] [geode] nabarunnag closed pull request #4070: GEODE-7218: Redundant addAll call.

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

[GitHub] [geode] kirklund opened pull request #4071: GEODE-7165: Remove networking dependency from GatewayReceiverImplTest

2019-09-19 Thread GitHub
The test was failing on machines with ports 2000 or 2001 in use. This change replaces GatewayReceiverImpl's dependency on AvailablePort with functions that are now injected by the unit tests. Specifically, this test was failing for @dickcav and @onichols-pivotal. [ Full content available at:

[GitHub] [geode] jujoramos closed pull request #3996: GEODE-7146: Improve Aggregate Function Docs

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

[GitHub] [geode] aaronlindsey commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

2019-09-19 Thread GitHub
> Should I remove them or keep them? As a general rule, I'd say adhere to the existing style. However, in Geode we don't seem to have a standard style for test names. I personally prefer the style with the underscores, but I'd be fine with it either way. [ Full content available at:

[GitHub] [geode] pivotal-jbarrett commented on issue #4003: GEODE-6964: Move geode log4j core classes to geode-log4j

2019-09-19 Thread GitHub
There is a lot going on here. I will preface that I haven't read the entire PR. I would prefer to have seen an RFC that described the intent of this PR. I believe the intent is go move all the Log4J specific code into its own module. This module is expected to be an SPI implementation of the

[GitHub] [geode] pivotal-jbarrett commented on issue #4003: GEODE-6964: Move geode log4j core classes to geode-log4j

2019-09-19 Thread GitHub
There is a lot going on here. I will preface that I haven't read the entire PR. I would prefer to have seen an RFC that described the intent of this PR. I believe the intent is to move all the Log4J specific code into its own module. This module is expected to be an SPI implementation of the

[GitHub] [geode] BenjaminPerryRoss commented on pull request #4061: Geode 6874: add unit tests to tomcat modules

2019-09-19 Thread GitHub
The problem is all these input streams are being created, used, and discarded internally in the load/unload process. This means that if we want to verify any behavior of those objects (in addition to avoiding the NullPointerExceptions that come with trying to actually perform these filesystem

[GitHub] [geode] bschuchardt opened pull request #4073: GEODE-7219 BufferUnderflowException in PutReplyMessage deserialization

2019-09-19 Thread GitHub
VersionTag serialization was being affected by concurrent modification of its memberId/previousMemberId fields, causing the HAS_PREVIOUS_MEMBER_ID flag bit to be set and the DUPLICATE_MEMBER_IDS flag to _not_ be set. It then went on to perform the same checks later in toData() and make different

[GitHub] [geode] BenjaminPerryRoss commented on pull request #4061: Geode 6874: add unit tests to tomcat modules

2019-09-19 Thread GitHub
In other tests this was possible and I've replaced internal uses there with the public interfaces. For this test specifically, the 3 internal references are necessary. The product itself performs a cast of the retrieved Cache object to GemFireCacheImpl, and as a result passing a mock of

[GitHub] [geode] kirklund opened pull request #4072: DRAFT: Do not review

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

[GitHub] [geode] alb3rtobr commented on issue #4067: GEODE-6871: Disk free space info exposed via JMX

2019-09-19 Thread GitHub
> Seems alright to me, I appreciate the descriptive test names! Its better to use descriptive names than comments explaining the test case :) Thanks! [ Full content available at: https://github.com/apache/geode/pull/4067 ] This message was relayed via gitbox.apache.org for

[GitHub] [geode] demery-pivotal commented on pull request #4072: GEODE-7221: Ensure management regions close CachePerfStats

2019-09-19 Thread GitHub
Please add tests showing that the LocalRegion closes cachePerfStats when the holder has own stats, and does not when the holder doesn't have own stats. [ Full content available at: https://github.com/apache/geode/pull/4072 ] This message was relayed via gitbox.apache.org for

[GitHub] [geode] Bill commented on pull request #4073: GEODE-7219 BufferUnderflowException in PutReplyMessage deserialization

2019-09-19 Thread GitHub
I'm curious why you chose to introduce a new variable rather than referencing the `DUPLICATE_MEMBER_IDS` bit in `flags`? [ Full content available at: https://github.com/apache/geode/pull/4073 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org