[GitHub] [geode] jdeppe-pivotal closed pull request #4393: GEODE-7521: Add slash to start of region name.

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

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #556: GEODE-7509: Fix memory leaks in C++ client

2019-12-03 Thread GitHub
Ok, I can reproduce the failure with string literal `u8"SimpleCQ\xF0\x90\x90\x80"` as the CQ name. This UTF-8 sequence produces the U+10400 character "Ѐ", which has two UTF-16 code points and results in 6 bytes of JMUTF-8 rather then 4 bytes of UTF-8. I also found a race condition in the new

[GitHub] [geode] jinmeiliao commented on pull request #4386: GEODE-7504: region eviction support

2019-12-03 Thread GitHub
I think the differences is the later approach would give user less opportunities to create an invalid object to begin with, whereas the the previous approach will let user create any combination of attributes and let validator to catch all the invalid cases. [ Full content available at:

[GitHub] [geode] davebarnes97 closed pull request #4390: GEODE-7454 Document Cluster Management Service (experimental)(

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

[GitHub] [geode-native] pdxcodemonkey commented on issue #558: GEODE-7520: Disable clang-format by default

2019-12-03 Thread GitHub
I respectfully disagree: * Just as a ballpark estimate, developers on the team spend several minutes of build time per day running clang-format on code. This doesn't strictly need to be done until it's time to push. * In point of fact, formatting is *not* enforced in the build, or at least not

[GitHub] [geode] dschneider-pivotal commented on pull request #4386: GEODE-7504: region eviction support

2019-12-03 Thread GitHub
add javadocs describing when it will throw also describe that it will also set the type to...l [ Full content available at: https://github.com/apache/geode/pull/4386 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mhansonp closed pull request #4375: Do Not Review: GEODE-6374 Changes to wait and single sourcing code.

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

[GitHub] [geode] mhansonp commented on issue #4375: Do Not Review: GEODE-6374 Changes to wait and single sourcing code.

2019-12-03 Thread GitHub
Don't want PR builds for the moment. [ Full content available at: https://github.com/apache/geode/pull/4375 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] rhoughton-pivot opened pull request #4402: Separate benchmark tasks into jobs.

2019-12-03 Thread GitHub
To enable more granular re-run in Concourse, split the Benchmarks into independently triggerable jobs in the main pipeline. Extract variables from the template into the jinja.variables file. Make better use of YML anchors. Co-authored-by: Robert Houghton Co-authored-by: Helena Bales [ Full

[GitHub] [geode-native] pivotal-jbarrett commented on issue #558: GEODE-7520: Disable clang-format by default

2019-12-03 Thread GitHub
> I'd be open to a solution that allowed this target or targets to be on by > default if they can a) be explicitly turned off via cmake variable and b) run > for the entire code base without doing backflips on the command line. Let me > know what you think. `$ cmake ...

[GitHub] [geode] kirklund opened pull request #4397: GEODE-7526: Add DistributedExecutorServiceRule for dunit tests

2019-12-03 Thread GitHub
JUnit Rule that provides an ExecutorService to each VM in a distributed test. See javadocs of both DistributedExecutorServiceRule and ExecutorServiceRule for more info. [ Full content available at: https://github.com/apache/geode/pull/4397 ] This message was relayed via gitbox.apache.org for

[GitHub] [geode] bschuchardt opened pull request #4398: GEODE-7519 GMSHealthMonitorJUnitTest fails in testCheckIfAvailableNoH…

2019-12-03 Thread GitHub
…eartBeatDontRemoveMember Removing time-sensitive test. There are other tests that already exercise the checkIfAvailable health-monitor method. Two of those tests make sure that the initiateRemoval parameter (the last parameter) to checkIfAvailable functions properly and that the value returned

[GitHub] [geode] Bill commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
This logic, to determine if a recipients array designates "all recipients" is both unusual and repeated. So I recommend it be put into its own method. Repeated here: * `ClusterDistributionManager.sendViaMembershipManager()` * `DistributionMessage.getRecipients()` we also have

[GitHub] [geode] Bill commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
bravissimo! [ Full content available at: https://github.com/apache/geode/pull/4384 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
`ALL_RECIPIENTS` should not be defined in this class because it's already defined up in the `Message` interface [ Full content available at: https://github.com/apache/geode/pull/4384 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] gemzdude opened pull request #4399: Feature/geode 7527

2019-12-03 Thread GitHub
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? - [ ]

[GitHub] [geode] rhoughton-pivot closed pull request #4391: rsync all inputs and outputs between the heavy-lifter and concourse

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

[GitHub] [geode] bschuchardt commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
The use of [null] to represent "forAll" was made many years ago and I don't want to mess with that just to remove this dependency. [ Full content available at: https://github.com/apache/geode/pull/4384 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
I missed that one - I'll address it in this ticket. [ Full content available at: https://github.com/apache/geode/pull/4384 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] echobravopapa commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
@Bill maybe make a ticket to improve this separate from this PR [ Full content available at: https://github.com/apache/geode/pull/4384 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] echobravopapa commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
Powerfull IntelliJ [ Full content available at: https://github.com/apache/geode/pull/4384 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] echobravopapa commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
good name change [ Full content available at: https://github.com/apache/geode/pull/4384 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] moleske opened pull request #4408: Fix LGTM InternalLocator Container Never Accessed Issue

2019-12-03 Thread GitHub
Along with other cleanup 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

[GitHub] [geode] rhoughton-pivot closed pull request #4402: Separate benchmark tasks into jobs.

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

[GitHub] [geode] Bill commented on pull request #4398: GEODE-7519 GMSHealthMonitorJUnitTest fails in testCheckIfAvailableNoH…

2019-12-03 Thread GitHub
Is the point of this test to verify that calling `checkIfAvailable(x,x,true)` under the following conditions, returns `false`: 1. no heartbeats have been received from the member 2. we have not yet reached the first timeout time Notwithstanding the reasoning in the description of this PR, I

[GitHub] [geode] bschuchardt commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
getRecipientsDescription ought to be changed but it is only used in logging [ Full content available at: https://github.com/apache/geode/pull/4384 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund opened pull request #4410: GEODE-7503: GemFireCacheImpl close blocks until close completes

2019-12-03 Thread GitHub
Subsequent calls to GemFireCacheImpl close will now block until the first call to close completes. Additional changes: * Expand unit testing of GemFireCacheImpl * Inject dependencies into GemFireCacheImpl constructor * Cleanup existing tests of GemFireCacheImpl [ Full content available at:

[GitHub] [geode] agingade commented on pull request #4366: GEODE-7478: Register interest in appropriate cases.

2019-12-03 Thread GitHub
We could registerInterest in a common place for both user and internally created session regions. [ Full content available at: https://github.com/apache/geode/pull/4366 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] agingade commented on pull request #4366: GEODE-7478: Register interest in appropriate cases.

2019-12-03 Thread GitHub
We could registerInterest in a common place for both user and internally created session regions. [ Full content available at: https://github.com/apache/geode/pull/4366 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jhuynh1 commented on pull request #4385: GEODE-7497: Check CQs prior to change authorizer

2019-12-03 Thread GitHub
Should this be pulled into a method? It would be self documenting (no comment needed) if the method name was correctly chosen [ Full content available at: https://github.com/apache/geode/pull/4385 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mhansonp opened pull request #4409: Geode 6374 - Fixing DistributedNoAckRegionCCEOffHeapDUnitTest.testNoLoaderWithInvalidEntry

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

[GitHub] [geode] upthewaterspout commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
DistributionImpl is part of geode-core. Should it be using AbstractGMSMessage (not part of the geode-membership API). [ Full content available at: https://github.com/apache/geode/pull/4384 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] upthewaterspout commented on pull request #4384: GEODE-7507 remove GMSMembership's dependency on DistributionMessage

2019-12-03 Thread GitHub
Shouldn't this be MemberIdentifier, not InternalDistributedMember? [ Full content available at: https://github.com/apache/geode/pull/4384 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] lgtm-com[bot] commented on issue #4411: Fix LGTM TXOriginatorRecoveryProcessor Container Never Accessed

2019-12-03 Thread lgtm-com
This pull request **fixes 1 alert** when merging fc37449ca2f88d0e8cdf185ddd07a40d1ea825c4 into c6081fa106697bf787e102f2f50a54ffdfcba726 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-b85c3d08033014a344231e0120e392239e9b3c58) **fixed alerts:** * 1 for Container contents

[GitHub] [geode] moleske opened pull request #4411: Fix LGTM TXOriginatorRecoveryProcessor Container Never Accessed

2019-12-03 Thread GitHub
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? - [x] Has

[GitHub] [geode] davebarnes97 opened pull request #4412: GEODE-7454 Document Cluster Mgmt Svc: introduce a doc template variable

2019-12-03 Thread GitHub
Tweak to the version variable that links to the Wiki feature description - needs to be geode-specific [ Full content available at: https://github.com/apache/geode/pull/4412 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] moleske opened pull request #4413: Fix LGTM JTAUtils Issues

2019-12-03 Thread GitHub
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

[GitHub] [geode] lgtm-com[bot] commented on issue #4413: Fix LGTM JTAUtils Issues

2019-12-03 Thread lgtm-com
This pull request **fixes 5 alerts** when merging edce714bf8fb9068270d05400f02d9d2194421d2 into c6081fa106697bf787e102f2f50a54ffdfcba726 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-5c4c93a09671be971b517ec31191bb1109f639c3) **fixed alerts:** * 2 for Potential database

[GitHub] [geode] mhansonp commented on pull request #4409: Geode 6374 - Fixing DistributedNoAckRegionCCEOffHeapDUnitTest.testNoLoaderWithInvalidEntry

2019-12-03 Thread GitHub
The bug. [ Full content available at: https://github.com/apache/geode/pull/4409 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org