[GitHub] geode issue #383: GEODE-2206: Add junit-quickcheck to geode-core.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/383 @scmbuildguy : done. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #383: GEODE-2206: Add junit-quickcheck to geode-core.
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/383 GEODE-2206: Add junit-quickcheck to geode-core. * Rewrite a data serialization test to use junit-quickcheck. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2206 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/383.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #383 commit 778f5802b6136637ef50f8ef506efb40021cc758 Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-01-31T23:05:17Z GEODE-2206: Add junit-quickcheck to geode-core. * Rewrite a data serialization test to use junit-quickcheck. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #368: [GEODE-2381] make complex enums more readable
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/368 @kirklund no problem, the change is what counts. ð --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #383: GEODE-2206: Add junit-quickcheck to geode-core.
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/383#discussion_r99265598
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/InternalDataSerializerQuickcheckStringTest.java
---
@@ -0,0 +1,63 @@
+/*
+ * 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.internal;
+
+import static org.junit.Assert.*;
+
+import com.pholser.junit.quickcheck.Property;
+import com.pholser.junit.quickcheck.runner.JUnitQuickcheck;
+import org.apache.geode.DataSerializer;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.junit.Before;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import java.io.ByteArrayOutputStream;
+import java.io.ByteArrayInputStream;
+import java.io.DataInputStream;
+import java.io.DataOutputStream;
+import java.io.IOException;
+
+/**
+ * Tests the serialization and deserialization of randomly generated
Strings.
+ *
+ * The current implementation (0.7 or 0.8alpha2) of
junit-quickcheck-generators only generates valid
+ * codepoints, and that it doesn't tend to test strings that are
particularly long, though the more
+ * trials you run, the longer they get.
+ */
+@Category(UnitTest.class)
+@RunWith(JUnitQuickcheck.class)
+public class InternalDataSerializerQuickcheckStringTest {
+ @Property(trials = 1000)
+ public void StringSerializedDeserializesToSameValue(String
originalString) throws IOException {
+ByteArrayOutputStream byteArrayOutputStream = new
ByteArrayOutputStream();
+DataOutputStream dataOutputStream = new
DataOutputStream(byteArrayOutputStream);
+
+DataSerializer.writeString(originalString, dataOutputStream);
+dataOutputStream.flush();
+
+byte[] stringBytes = byteArrayOutputStream.toByteArray();
+DataInputStream dataInputStream = new DataInputStream(new
ByteArrayInputStream(stringBytes));
+String returnedString = DataSerializer.readString(dataInputStream);
+
+assertEquals("Deserialized string matches original", originalString,
returnedString);
--- End diff --
I think that would require rewriting either the test runner or the
assertion functions. I don't see the seed when I'm running in IntelliJ (though
it doesn't seem to discard output), but I do when I run the test suite (where
unprintable input appears as question marks):
```
shrinking: Deserialized string matches original expected:<[foo]> but
was:<[??
]>
Shrunken args: [??
]
Original failure message: [Deserialized string matches original
expected:<[foo]> but was:<[]>]
Original args: []
Seeds: [5559329435085103409]
```
I can then rerun the test with:
```
public void
StringSerializedDeserializesToSameValue(@When(seed=5559329435085103409L) String
originalString) throws IOException {
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #392: [GEODE-2414] Ignore a consistently failing test.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/392 @kirklund I'm not a committer; you're welcome to commit this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #392: [GEODE-2414] Ignore a consistently failing test.
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/392 [GEODE-2414] Ignore a consistently failing test. This test only seems to be failing on Linux. I haven't figured out the cause yet. Ignore it until I have time to fix it properly. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2412 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/392.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #392 commit c8056cae6c599ff08a6e2c968f76a95ca0fac58b Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-02-06T20:28:56Z [GEODE-2414] Ignore a consistently failing test. This test only seems to be failing on Linux. I haven't figured out the cause yet. Ignore it until I have time to fix it properly. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #383: GEODE-2206: Add junit-quickcheck to geode-core.
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/383#discussion_r99371844
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/InternalDataSerializerQuickcheckStringTest.java
---
@@ -0,0 +1,63 @@
+/*
+ * 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.internal;
+
+import static org.junit.Assert.*;
+
+import com.pholser.junit.quickcheck.Property;
+import com.pholser.junit.quickcheck.runner.JUnitQuickcheck;
+import org.apache.geode.DataSerializer;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.junit.Before;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import java.io.ByteArrayOutputStream;
+import java.io.ByteArrayInputStream;
+import java.io.DataInputStream;
+import java.io.DataOutputStream;
+import java.io.IOException;
+
+/**
+ * Tests the serialization and deserialization of randomly generated
Strings.
+ *
+ * The current implementation (0.7 or 0.8alpha2) of
junit-quickcheck-generators only generates valid
+ * codepoints, and that it doesn't tend to test strings that are
particularly long, though the more
+ * trials you run, the longer they get.
+ */
+@Category(UnitTest.class)
+@RunWith(JUnitQuickcheck.class)
+public class InternalDataSerializerQuickcheckStringTest {
+ @Property(trials = 1000)
+ public void StringSerializedDeserializesToSameValue(String
originalString) throws IOException {
+ByteArrayOutputStream byteArrayOutputStream = new
ByteArrayOutputStream();
+DataOutputStream dataOutputStream = new
DataOutputStream(byteArrayOutputStream);
+
+DataSerializer.writeString(originalString, dataOutputStream);
+dataOutputStream.flush();
+
+byte[] stringBytes = byteArrayOutputStream.toByteArray();
+DataInputStream dataInputStream = new DataInputStream(new
ByteArrayInputStream(stringBytes));
+String returnedString = DataSerializer.readString(dataInputStream);
+
+assertEquals("Deserialized string matches original", originalString,
returnedString);
--- End diff --
Yeah, I'm not sure why IntelliJ doesn't show the seed. I remember there
being a way to run int tests on *all* inputs. I don't see it in the docs now,
but you could write your own generator and run with that to do exhaustive
testing of something that you knew failed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #368: [GEODE-2381] make complex enums more readable
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/368 I don't really understand what the `48` means, but it's used elsewhere and looks something like "put everything on its own line". --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #313: [GEODE-1407] Change a FlakyTest to distributedTest.
Github user galen-pivotal closed the pull request at: https://github.com/apache/geode/pull/313 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #368: [GEODE-2381] make complex enums not look terrible.
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/368 [GEODE-2381] make complex enums not look terrible. * Change the style XML file to insert a line break after each constant of an enum. There may be a better way to do this and allow simple enums to be all on one line, but this looks better than what we have to me. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2381 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/368.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #368 commit 64039a80d6d73b155c892f96ea4a96fedda6def2 Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-01-27T22:19:49Z [GEODE-2381] spotless: line break on enum constants. * Change the style XML file to insert a line break after each constant of an enum. * This commit is just the style file change; spotless will be applied next. There may be a better way to do this and allow simple enums to be all on one line, but this looks better to me. commit d98a656b0c0c691a17b8a61d0f13643b3e075308 Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-01-27T22:21:01Z [GEODE-2381] Apply spotless to fix enums and nothing else in this commit. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #402: GEODE-2497 surprise members are never timed out dur...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/402#discussion_r102357674
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionManagerDUnitTest.java
---
@@ -159,77 +159,74 @@ public void testConnectAfterBeingShunned() {
**/
@Test
public void testSurpriseMemberHandling() {
-VM vm0 = Host.getHost(0).getVM(0);
-
-InternalDistributedSystem sys = getSystem();
-MembershipManager mgr =
MembershipManagerHelper.getMembershipManager(sys);
+System.setProperty(DistributionConfig.GEMFIRE_PREFIX +
"surprise-member-timeout", "3000");
try {
- InternalDistributedMember mbr =
- new InternalDistributedMember(NetworkUtils.getIPLiteral(),
12345);
+ InternalDistributedSystem sys = getSystem();
+ MembershipManager mgr =
MembershipManagerHelper.getMembershipManager(sys);
+ assertTrue(((GMSMembershipManager) mgr).isCleanupTimerStarted());
- // first make sure we can't add this as a surprise member (bug
#44566)
-
- // if the view number isn't being recorded correctly the test will
pass but the
- // functionality is broken
- Assert.assertTrue("expected view ID to be greater than zero",
mgr.getView().getViewId() > 0);
-
- int oldViewId = mbr.getVmViewId();
- mbr.setVmViewId((int) mgr.getView().getViewId() - 1);
- org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
- .info("current membership view is " + mgr.getView());
- org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
- .info("created ID " + mbr + " with view ID " +
mbr.getVmViewId());
- sys.getLogWriter()
- .info("attempt to add old
member");
- sys.getLogWriter()
- .info("Removing shunned GemFire
node");
try {
-boolean accepted = mgr.addSurpriseMember(mbr);
-Assert.assertTrue("member with old ID was not rejected (bug
#44566)", !accepted);
- } finally {
+InternalDistributedMember mbr =
+new InternalDistributedMember(NetworkUtils.getIPLiteral(),
12345);
+
+// first make sure we can't add this as a surprise member (bug
#44566)
+
+// if the view number isn't being recorded correctly the test will
pass but the
+// functionality is broken
+Assert.assertTrue("expected view ID to be greater than zero",
+mgr.getView().getViewId() > 0);
+
+int oldViewId = mbr.getVmViewId();
+mbr.setVmViewId((int) mgr.getView().getViewId() - 1);
+org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+.info("current membership view is " + mgr.getView());
+org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+.info("created ID " + mbr + " with view ID " +
mbr.getVmViewId());
sys.getLogWriter()
-.info("attempt to add old
member");
+.info("attempt to add old
member");
sys.getLogWriter().info(
-"Removing shunned GemFire
node");
- }
- mbr.setVmViewId(oldViewId);
-
- // now forcibly add it as a surprise member and show that it is
reaped
- long gracePeriod = 5000;
- long startTime = System.currentTimeMillis();
- long timeout = ((GMSMembershipManager)
mgr).getSurpriseMemberTimeout();
- long birthTime = startTime - timeout + gracePeriod;
- MembershipManagerHelper.addSurpriseMember(sys, mbr, birthTime);
- assertTrue("Member was not a surprise member",
mgr.isSurpriseMember(mbr));
-
- // force a real view change
- SerializableRunnable connectDisconnect = new SerializableRunnable() {
-public void run() {
- getSystem().disconnect();
+"Removing shunned GemFire
node");
+try {
+ boolean accepted = mgr.addSurpriseMember(mbr);
+ Assert.assertTrue("member with old ID was not rejected (bug
#44566)", !accepted);
+} finally {
+ sys.getLogWriter().info(
+ "attempt to add old
member");
--- End diff --
Ah, so this would be the equivalent call now?
```java
IgnoredException.addIgnoredException("attempt to add old member");
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #402: GEODE-2497 surprise members are never timed out dur...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/402#discussion_r102345903
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionManagerDUnitTest.java
---
@@ -159,77 +159,74 @@ public void testConnectAfterBeingShunned() {
**/
@Test
public void testSurpriseMemberHandling() {
-VM vm0 = Host.getHost(0).getVM(0);
-
-InternalDistributedSystem sys = getSystem();
-MembershipManager mgr =
MembershipManagerHelper.getMembershipManager(sys);
+System.setProperty(DistributionConfig.GEMFIRE_PREFIX +
"surprise-member-timeout", "3000");
try {
- InternalDistributedMember mbr =
- new InternalDistributedMember(NetworkUtils.getIPLiteral(),
12345);
+ InternalDistributedSystem sys = getSystem();
+ MembershipManager mgr =
MembershipManagerHelper.getMembershipManager(sys);
+ assertTrue(((GMSMembershipManager) mgr).isCleanupTimerStarted());
- // first make sure we can't add this as a surprise member (bug
#44566)
-
- // if the view number isn't being recorded correctly the test will
pass but the
- // functionality is broken
- Assert.assertTrue("expected view ID to be greater than zero",
mgr.getView().getViewId() > 0);
-
- int oldViewId = mbr.getVmViewId();
- mbr.setVmViewId((int) mgr.getView().getViewId() - 1);
- org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
- .info("current membership view is " + mgr.getView());
- org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
- .info("created ID " + mbr + " with view ID " +
mbr.getVmViewId());
- sys.getLogWriter()
- .info("attempt to add old
member");
- sys.getLogWriter()
- .info("Removing shunned GemFire
node");
try {
-boolean accepted = mgr.addSurpriseMember(mbr);
-Assert.assertTrue("member with old ID was not rejected (bug
#44566)", !accepted);
- } finally {
+InternalDistributedMember mbr =
+new InternalDistributedMember(NetworkUtils.getIPLiteral(),
12345);
+
+// first make sure we can't add this as a surprise member (bug
#44566)
+
+// if the view number isn't being recorded correctly the test will
pass but the
+// functionality is broken
+Assert.assertTrue("expected view ID to be greater than zero",
+mgr.getView().getViewId() > 0);
+
+int oldViewId = mbr.getVmViewId();
+mbr.setVmViewId((int) mgr.getView().getViewId() - 1);
+org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+.info("current membership view is " + mgr.getView());
+org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+.info("created ID " + mbr + " with view ID " +
mbr.getVmViewId());
sys.getLogWriter()
-.info("attempt to add old
member");
+.info("attempt to add old
member");
sys.getLogWriter().info(
-"Removing shunned GemFire
node");
- }
- mbr.setVmViewId(oldViewId);
-
- // now forcibly add it as a surprise member and show that it is
reaped
- long gracePeriod = 5000;
- long startTime = System.currentTimeMillis();
- long timeout = ((GMSMembershipManager)
mgr).getSurpriseMemberTimeout();
- long birthTime = startTime - timeout + gracePeriod;
- MembershipManagerHelper.addSurpriseMember(sys, mbr, birthTime);
- assertTrue("Member was not a surprise member",
mgr.isSurpriseMember(mbr));
-
- // force a real view change
- SerializableRunnable connectDisconnect = new SerializableRunnable() {
-public void run() {
- getSystem().disconnect();
+"Removing shunned GemFire
node");
+try {
+ boolean accepted = mgr.addSurpriseMember(mbr);
+ Assert.assertTrue("member with old ID was not rejected (bug
#44566)", !accepted);
+} finally {
+ sys.getLogWriter().info(
+ "attempt to add old
member");
+ sys.getLogWriter().info(
+ "Removing shunned GemFire
node");
}
- };
- vm0.invoke(connectDisconnect);
-
- if (birthTime < (System.currentTimeMillis() - timeout)) {
-return; // machine is too busy and we didn't get enough CPU to
perform more assertions
- }
- assertTrue("Member w
[GitHub] geode pull request #402: GEODE-2497 surprise members are never timed out dur...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/402#discussion_r102345774
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionManagerDUnitTest.java
---
@@ -159,77 +159,74 @@ public void testConnectAfterBeingShunned() {
**/
@Test
public void testSurpriseMemberHandling() {
-VM vm0 = Host.getHost(0).getVM(0);
-
-InternalDistributedSystem sys = getSystem();
-MembershipManager mgr =
MembershipManagerHelper.getMembershipManager(sys);
+System.setProperty(DistributionConfig.GEMFIRE_PREFIX +
"surprise-member-timeout", "3000");
try {
- InternalDistributedMember mbr =
- new InternalDistributedMember(NetworkUtils.getIPLiteral(),
12345);
+ InternalDistributedSystem sys = getSystem();
+ MembershipManager mgr =
MembershipManagerHelper.getMembershipManager(sys);
+ assertTrue(((GMSMembershipManager) mgr).isCleanupTimerStarted());
- // first make sure we can't add this as a surprise member (bug
#44566)
-
- // if the view number isn't being recorded correctly the test will
pass but the
- // functionality is broken
- Assert.assertTrue("expected view ID to be greater than zero",
mgr.getView().getViewId() > 0);
-
- int oldViewId = mbr.getVmViewId();
- mbr.setVmViewId((int) mgr.getView().getViewId() - 1);
- org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
- .info("current membership view is " + mgr.getView());
- org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
- .info("created ID " + mbr + " with view ID " +
mbr.getVmViewId());
- sys.getLogWriter()
- .info("attempt to add old
member");
- sys.getLogWriter()
- .info("Removing shunned GemFire
node");
try {
-boolean accepted = mgr.addSurpriseMember(mbr);
-Assert.assertTrue("member with old ID was not rejected (bug
#44566)", !accepted);
- } finally {
+InternalDistributedMember mbr =
+new InternalDistributedMember(NetworkUtils.getIPLiteral(),
12345);
+
+// first make sure we can't add this as a surprise member (bug
#44566)
+
+// if the view number isn't being recorded correctly the test will
pass but the
+// functionality is broken
+Assert.assertTrue("expected view ID to be greater than zero",
+mgr.getView().getViewId() > 0);
+
+int oldViewId = mbr.getVmViewId();
+mbr.setVmViewId((int) mgr.getView().getViewId() - 1);
+org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+.info("current membership view is " + mgr.getView());
+org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+.info("created ID " + mbr + " with view ID " +
mbr.getVmViewId());
sys.getLogWriter()
-.info("attempt to add old
member");
+.info("attempt to add old
member");
sys.getLogWriter().info(
-"Removing shunned GemFire
node");
- }
- mbr.setVmViewId(oldViewId);
-
- // now forcibly add it as a surprise member and show that it is
reaped
- long gracePeriod = 5000;
- long startTime = System.currentTimeMillis();
- long timeout = ((GMSMembershipManager)
mgr).getSurpriseMemberTimeout();
- long birthTime = startTime - timeout + gracePeriod;
- MembershipManagerHelper.addSurpriseMember(sys, mbr, birthTime);
- assertTrue("Member was not a surprise member",
mgr.isSurpriseMember(mbr));
-
- // force a real view change
- SerializableRunnable connectDisconnect = new SerializableRunnable() {
-public void run() {
- getSystem().disconnect();
+"Removing shunned GemFire
node");
+try {
+ boolean accepted = mgr.addSurpriseMember(mbr);
+ Assert.assertTrue("member with old ID was not rejected (bug
#44566)", !accepted);
+} finally {
+ sys.getLogWriter().info(
+ "attempt to add old
member");
+ sys.getLogWriter().info(
+ "Removing shunned GemFire
node");
}
- };
- vm0.invoke(connectDisconnect);
-
- if (birthTime < (System.currentTimeMillis() - timeout)) {
-return; // machine is too busy and we didn't get enough CPU to
perform more assertions
- }
- assertTrue("Member w
[GitHub] geode pull request #402: GEODE-2497 surprise members are never timed out dur...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/402#discussion_r102345794
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionManagerDUnitTest.java
---
@@ -159,77 +159,74 @@ public void testConnectAfterBeingShunned() {
**/
@Test
public void testSurpriseMemberHandling() {
-VM vm0 = Host.getHost(0).getVM(0);
-
-InternalDistributedSystem sys = getSystem();
-MembershipManager mgr =
MembershipManagerHelper.getMembershipManager(sys);
+System.setProperty(DistributionConfig.GEMFIRE_PREFIX +
"surprise-member-timeout", "3000");
try {
- InternalDistributedMember mbr =
- new InternalDistributedMember(NetworkUtils.getIPLiteral(),
12345);
+ InternalDistributedSystem sys = getSystem();
+ MembershipManager mgr =
MembershipManagerHelper.getMembershipManager(sys);
+ assertTrue(((GMSMembershipManager) mgr).isCleanupTimerStarted());
- // first make sure we can't add this as a surprise member (bug
#44566)
-
- // if the view number isn't being recorded correctly the test will
pass but the
- // functionality is broken
- Assert.assertTrue("expected view ID to be greater than zero",
mgr.getView().getViewId() > 0);
-
- int oldViewId = mbr.getVmViewId();
- mbr.setVmViewId((int) mgr.getView().getViewId() - 1);
- org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
- .info("current membership view is " + mgr.getView());
- org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
- .info("created ID " + mbr + " with view ID " +
mbr.getVmViewId());
- sys.getLogWriter()
- .info("attempt to add old
member");
- sys.getLogWriter()
- .info("Removing shunned GemFire
node");
try {
-boolean accepted = mgr.addSurpriseMember(mbr);
-Assert.assertTrue("member with old ID was not rejected (bug
#44566)", !accepted);
- } finally {
+InternalDistributedMember mbr =
+new InternalDistributedMember(NetworkUtils.getIPLiteral(),
12345);
+
+// first make sure we can't add this as a surprise member (bug
#44566)
+
+// if the view number isn't being recorded correctly the test will
pass but the
+// functionality is broken
+Assert.assertTrue("expected view ID to be greater than zero",
+mgr.getView().getViewId() > 0);
+
+int oldViewId = mbr.getVmViewId();
+mbr.setVmViewId((int) mgr.getView().getViewId() - 1);
+org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+.info("current membership view is " + mgr.getView());
+org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+.info("created ID " + mbr + " with view ID " +
mbr.getVmViewId());
sys.getLogWriter()
-.info("attempt to add old
member");
+.info("attempt to add old
member");
sys.getLogWriter().info(
-"Removing shunned GemFire
node");
- }
- mbr.setVmViewId(oldViewId);
-
- // now forcibly add it as a surprise member and show that it is
reaped
- long gracePeriod = 5000;
- long startTime = System.currentTimeMillis();
- long timeout = ((GMSMembershipManager)
mgr).getSurpriseMemberTimeout();
- long birthTime = startTime - timeout + gracePeriod;
- MembershipManagerHelper.addSurpriseMember(sys, mbr, birthTime);
- assertTrue("Member was not a surprise member",
mgr.isSurpriseMember(mbr));
-
- // force a real view change
- SerializableRunnable connectDisconnect = new SerializableRunnable() {
-public void run() {
- getSystem().disconnect();
+"Removing shunned GemFire
node");
+try {
+ boolean accepted = mgr.addSurpriseMember(mbr);
+ Assert.assertTrue("member with old ID was not rejected (bug
#44566)", !accepted);
+} finally {
+ sys.getLogWriter().info(
+ "attempt to add old
member");
+ sys.getLogWriter().info(
+ "Removing shunned GemFire
node");
}
- };
- vm0.invoke(connectDisconnect);
-
- if (birthTime < (System.currentTimeMillis() - timeout)) {
-return; // machine is too busy and we didn't get enough CPU to
perform more assertions
- }
- assertTrue("Member w
[GitHub] geode pull request #398: Split the redis adapter into its own package
Github user galen-pivotal closed the pull request at: https://github.com/apache/geode/pull/398 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #398: Split the redis adapter into its own package
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/398 We're now running this as a feature branch for the current round of Redis work, `feature/GEODE-2444`. I'm going to close this PR for now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #400: Add geode-redis to assembly.
GitHub user galen-pivotal reopened a pull request: https://github.com/apache/geode/pull/400 Add geode-redis to assembly. This makes starting a server with Redis actually work, at least on my machine. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2444 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/400.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #400 commit 3f9e52534a23957744a42487a7f0255dd50f3657 Author: Galen OSullivan <gosulli...@pivotal.io> Date: 2017-02-15T21:55:19Z Add geode-redis to assembly. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #400: Add geode-redis to assembly.
Github user galen-pivotal closed the pull request at: https://github.com/apache/geode/pull/400 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #400: Add geode-redis to assembly.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/400 It doesn't look like it, but this has been merged. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #402: GEODE-2497 surprise members are never timed out dur...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/402#discussion_r101873819
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionManagerDUnitTest.java
---
@@ -159,77 +159,74 @@ public void testConnectAfterBeingShunned() {
**/
@Test
public void testSurpriseMemberHandling() {
-VM vm0 = Host.getHost(0).getVM(0);
-
-InternalDistributedSystem sys = getSystem();
-MembershipManager mgr =
MembershipManagerHelper.getMembershipManager(sys);
+System.setProperty(DistributionConfig.GEMFIRE_PREFIX +
"surprise-member-timeout", "3000");
try {
- InternalDistributedMember mbr =
- new InternalDistributedMember(NetworkUtils.getIPLiteral(),
12345);
+ InternalDistributedSystem sys = getSystem();
+ MembershipManager mgr =
MembershipManagerHelper.getMembershipManager(sys);
+ assertTrue(((GMSMembershipManager) mgr).isCleanupTimerStarted());
- // first make sure we can't add this as a surprise member (bug
#44566)
-
- // if the view number isn't being recorded correctly the test will
pass but the
- // functionality is broken
- Assert.assertTrue("expected view ID to be greater than zero",
mgr.getView().getViewId() > 0);
-
- int oldViewId = mbr.getVmViewId();
- mbr.setVmViewId((int) mgr.getView().getViewId() - 1);
- org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
- .info("current membership view is " + mgr.getView());
- org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
- .info("created ID " + mbr + " with view ID " +
mbr.getVmViewId());
- sys.getLogWriter()
- .info("attempt to add old
member");
- sys.getLogWriter()
- .info("Removing shunned GemFire
node");
try {
-boolean accepted = mgr.addSurpriseMember(mbr);
-Assert.assertTrue("member with old ID was not rejected (bug
#44566)", !accepted);
- } finally {
+InternalDistributedMember mbr =
+new InternalDistributedMember(NetworkUtils.getIPLiteral(),
12345);
+
+// first make sure we can't add this as a surprise member (bug
#44566)
+
+// if the view number isn't being recorded correctly the test will
pass but the
+// functionality is broken
+Assert.assertTrue("expected view ID to be greater than zero",
+mgr.getView().getViewId() > 0);
+
+int oldViewId = mbr.getVmViewId();
+mbr.setVmViewId((int) mgr.getView().getViewId() - 1);
+org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+.info("current membership view is " + mgr.getView());
+org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+.info("created ID " + mbr + " with view ID " +
mbr.getVmViewId());
sys.getLogWriter()
-.info("attempt to add old
member");
+.info("attempt to add old
member");
sys.getLogWriter().info(
-"Removing shunned GemFire
node");
- }
- mbr.setVmViewId(oldViewId);
-
- // now forcibly add it as a surprise member and show that it is
reaped
- long gracePeriod = 5000;
- long startTime = System.currentTimeMillis();
- long timeout = ((GMSMembershipManager)
mgr).getSurpriseMemberTimeout();
- long birthTime = startTime - timeout + gracePeriod;
- MembershipManagerHelper.addSurpriseMember(sys, mbr, birthTime);
- assertTrue("Member was not a surprise member",
mgr.isSurpriseMember(mbr));
-
- // force a real view change
- SerializableRunnable connectDisconnect = new SerializableRunnable() {
-public void run() {
- getSystem().disconnect();
+"Removing shunned GemFire
node");
+try {
+ boolean accepted = mgr.addSurpriseMember(mbr);
+ Assert.assertTrue("member with old ID was not rejected (bug
#44566)", !accepted);
+} finally {
+ sys.getLogWriter().info(
+ "attempt to add old
member");
--- End diff --
Is this the old method of adding expected exceptions?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #349: [GEODE-2324] fixes to AcceptorImpl.close()
Github user galen-pivotal closed the pull request at: https://github.com/apache/geode/pull/349 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #349: [GEODE-2324] fixes to AcceptorImpl.close()
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/349 merged in cd2291102744f4bb1b8bb5170e7388b31295ef6f --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #359: [GEODE-2295] Add constructors for `LocatorCancelException`...
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/359 Merged in a61c55b9b0d4fffc70a1502f06cbca59098c9418 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #359: [GEODE-2295] Add constructors for `LocatorCancelExc...
Github user galen-pivotal closed the pull request at: https://github.com/apache/geode/pull/359 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #398: Split the redis adapter into its own package
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/398#discussion_r101374277
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
---
@@ -1330,21 +1360,12 @@ private void startMemcachedServer() {
}
private void startRedisServer() {
-int port = system.getConfig().getRedisPort();
-if (port != 0) {
- String bindAddress = system.getConfig().getRedisBindAddress();
- assert bindAddress != null;
- if
(bindAddress.equals(DistributionConfig.DEFAULT_REDIS_BIND_ADDRESS)) {
-getLoggerI18n().info(
-
LocalizedStrings.GemFireCacheImpl_STARTING_GEMFIRE_REDIS_SERVER_ON_PORT_0,
-new Object[] {port});
- } else {
-getLoggerI18n().info(
-
LocalizedStrings.GemFireCacheImpl_STARTING_GEMFIRE_REDIS_SERVER_ON_BIND_ADDRESS_0_PORT_1,
-new Object[] {bindAddress, port});
- }
- this.redisServer = new GeodeRedisServer(bindAddress, port);
- this.redisServer.start();
+GeodeRedisService geodeRedisService =
getService(GeodeRedisService.class);
--- End diff --
@metatype The plan is to have the Redis server registered as a service that
`GemFireCacheImpl` will start when the right properties are set.
`GeodeRedisService` is in core, but implemented by `GeodeRedisServiceImpl` as
long as geode-redis is on the classpath.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #398: Split the redis adapter into its own package
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/398 Split the redis adapter into its own package Under this PR, the redis adapter is moved to its own source root, and registered as a service. We're intending to make this a feature branch to improve the redis adapter. @bschuchardt You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2449 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/398.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #398 commit c6dbc6d4e4ea82d65074e30c4c15085a5e3d8688 Author: Udo Kohlmeyer <ukohlme...@pivotal.io> Date: 2017-02-10T21:33:31Z GEODE-2449: Moved Redis out of core with minimal Extension work added commit 81e64a9a27c74ffcff78103b66cc9ca9ec4f7cf3 Author: Udo Kohlmeyer <ukohlme...@pivotal.io> Date: 2017-02-10T21:43:28Z GEODE-2449: spotless commit 318b56a5e6a8cb443929dbe3d80fa5711777c2ef Author: Udo Kohlmeyer <ukohlme...@pivotal.io> Date: 2017-02-10T22:45:45Z GEODE-2449: Moved Coder.java from core to redis module. fix up code from code review commit 5562547e065906c3c1815875f8693ad0f4be93d0 Author: Udo Kohlmeyer <ukohlme...@pivotal.io> Date: 2017-02-10T23:21:27Z GEODE-2449: Do a null check on the stopRedisServer commit f79beb1e9d5ec08d25bb02f93eb96468b3253a72 Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-02-13T18:53:05Z GEODE-2449: changes in response to review. * Move HyperLogLog back into geode-core. * Bring back deprecated GeodeRedisServer for backwards compatibilty. commit c2c5e07eb8ad7ae55a24f5d972678b78852e0a15 Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-02-13T23:53:16Z Merge branch 'develop' into feature/GEODE-2449 commit 452ac17c90a48d13342ec25aa768aaa1e8359867 Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-02-14T00:29:13Z Don't throw an error if Redis isn't supposed to start. commit f990e7907cddad5522eea0b6919702489be7a49d Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-02-14T01:25:06Z update doc comment links commit a1116189eb25c2209e21685386d9acfcf6fbdb9e Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-02-14T07:48:01Z GEODE-2449. Don't throw exception on redis port of zero when Redis service is not found. We would like to have the ability to tell the difference between settings that are unset and those that are set to zero, but for the moment we can't. So that's how it is. It may be that we'll have to not allow starting a Redis host by setting a port number of zero in config, or that it will just be less reliable. commit 4ce902fce6759aed67bc1c321096326b7ce8bd60 Author: Galen OSullivan <gosulli...@pivotal.io> Date: 2017-02-14T17:22:03Z Fix a log message that was causing tests to fail. commit 2fbdb0cd8958051cb807a1674bf8344c92473802 Author: Galen OSullivan <gosulli...@pivotal.io> Date: 2017-02-14T22:30:56Z GEODE-2449. Don't expect moved Redis classes in core. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #399: Minor non-functional changes in response to PR comm...
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/399 Minor non-functional changes in response to PR comments. @bschuchardt You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2444 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/399.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #399 commit 742f5037a8644314c576577cf8ed8f66b2dafe9c Author: Galen OSullivan <gosulli...@pivotal.io> Date: 2017-02-15T18:53:13Z Minor non-functional changes in response to PR comments. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #398: Split the redis adapter into its own package
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/398#discussion_r101388974
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
---
@@ -1330,21 +1360,12 @@ private void startMemcachedServer() {
}
private void startRedisServer() {
-int port = system.getConfig().getRedisPort();
-if (port != 0) {
- String bindAddress = system.getConfig().getRedisBindAddress();
- assert bindAddress != null;
- if
(bindAddress.equals(DistributionConfig.DEFAULT_REDIS_BIND_ADDRESS)) {
-getLoggerI18n().info(
-
LocalizedStrings.GemFireCacheImpl_STARTING_GEMFIRE_REDIS_SERVER_ON_PORT_0,
-new Object[] {port});
- } else {
-getLoggerI18n().info(
-
LocalizedStrings.GemFireCacheImpl_STARTING_GEMFIRE_REDIS_SERVER_ON_BIND_ADDRESS_0_PORT_1,
-new Object[] {bindAddress, port});
- }
- this.redisServer = new GeodeRedisServer(bindAddress, port);
- this.redisServer.start();
+GeodeRedisService geodeRedisService =
getService(GeodeRedisService.class);
--- End diff --
Also, whatever code gfsh uses to start the Redis adapter will have to stay
in gfsh, unless there is a way to register extension commands with gfsh through
some not-yet-existent method. This would be great, though, because we could
plug in extensions that would change Geode's functionality.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #400: Add geode-redis to assembly.
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/400 Add geode-redis to assembly. This makes starting a server with Redis actually work, at least on my machine. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2444 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/400.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #400 commit 3f9e52534a23957744a42487a7f0255dd50f3657 Author: Galen OSullivan <gosulli...@pivotal.io> Date: 2017-02-15T21:55:19Z Add geode-redis to assembly. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #398: Split the redis adapter into its own package
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/398#discussion_r101388584
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
---
@@ -1330,21 +1360,12 @@ private void startMemcachedServer() {
}
private void startRedisServer() {
-int port = system.getConfig().getRedisPort();
-if (port != 0) {
- String bindAddress = system.getConfig().getRedisBindAddress();
- assert bindAddress != null;
- if
(bindAddress.equals(DistributionConfig.DEFAULT_REDIS_BIND_ADDRESS)) {
-getLoggerI18n().info(
-
LocalizedStrings.GemFireCacheImpl_STARTING_GEMFIRE_REDIS_SERVER_ON_PORT_0,
-new Object[] {port});
- } else {
-getLoggerI18n().info(
-
LocalizedStrings.GemFireCacheImpl_STARTING_GEMFIRE_REDIS_SERVER_ON_BIND_ADDRESS_0_PORT_1,
-new Object[] {bindAddress, port});
- }
- this.redisServer = new GeodeRedisServer(bindAddress, port);
- this.redisServer.start();
+GeodeRedisService geodeRedisService =
getService(GeodeRedisService.class);
--- End diff --
@metatype I suppose the exception call is mostly there because it's the
flag for whether Redis is enabled. I think that eventually we would like to
have the services be more modular (an "extension framework") and maybe start
them without even checking which interfaces they implement, but I don't think
that's fully formed yet.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #398: Split the redis adapter into its own package
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/398#discussion_r101401136
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
---
@@ -1330,21 +1360,12 @@ private void startMemcachedServer() {
}
private void startRedisServer() {
-int port = system.getConfig().getRedisPort();
-if (port != 0) {
- String bindAddress = system.getConfig().getRedisBindAddress();
- assert bindAddress != null;
- if
(bindAddress.equals(DistributionConfig.DEFAULT_REDIS_BIND_ADDRESS)) {
-getLoggerI18n().info(
-
LocalizedStrings.GemFireCacheImpl_STARTING_GEMFIRE_REDIS_SERVER_ON_PORT_0,
-new Object[] {port});
- } else {
-getLoggerI18n().info(
-
LocalizedStrings.GemFireCacheImpl_STARTING_GEMFIRE_REDIS_SERVER_ON_BIND_ADDRESS_0_PORT_1,
-new Object[] {bindAddress, port});
- }
- this.redisServer = new GeodeRedisServer(bindAddress, port);
- this.redisServer.start();
+GeodeRedisService geodeRedisService =
getService(GeodeRedisService.class);
--- End diff --
That's neat. Thanks for the heads up. I'll take a look when we get to fully
extensionizing it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #398: Split the redis adapter into its own package
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/398#discussion_r101354076
--- Diff:
geode-redis/src/test/java/org/apache/geode/redis/AuthJUnitTest.java ---
@@ -0,0 +1,120 @@
+/*
+ * 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.redis;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.cache.GemFireCacheImpl;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.Properties;
+import redis.clients.jedis.Jedis;
+import redis.clients.jedis.exceptions.JedisConnectionException;
+import redis.clients.jedis.exceptions.JedisDataException;
+
+@Category(IntegrationTest.class)
+public class AuthJUnitTest extends RedisTestBase {
+
+ private static final String PASSWORD = "pwd";
+
+ private void setupCacheWithPassword() {
+if (cache != null) {
+ cache.close();
+}
+Properties redisCacheProperties = getDefaultRedisCacheProperties();
+
redisCacheProperties.setProperty(ConfigurationProperties.REDIS_PASSWORD,
PASSWORD);
+cache = (GemFireCacheImpl) createCacheInstance(redisCacheProperties);
+ }
+
+ @Test
+ public void testAuthConfig() {
+setupCacheWithPassword();
+InternalDistributedSystem distributedSystem =
cache.getDistributedSystem();
+assert
(distributedSystem.getConfig().getRedisPassword().equals(PASSWORD));
+cache.close();
+ }
+
+ @Test
+ public void testAuthRejectAccept() {
+setupCacheWithPassword();
+try (Jedis jedis = defaultJedisInstance()) {
+ Exception ex = null;
+ try {
+jedis.auth("wrongpwd");
+ } catch (JedisDataException e) {
+ex = e;
+ }
+ assertNotNull(ex);
+
+ String res = jedis.auth(PASSWORD);
+ assertEquals(res, "OK");
+}
+ }
+
+ @Test
+ public void testAuthNoPwd() {
+try (Jedis jedis = defaultJedisInstance()) {
+ jedis.auth(PASSWORD);
+ fail(
+ "We expecting either a JedisConnectionException or
JedisDataException to be thrown here");
--- End diff --
I made the comment more informative to boot.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #398: Split the redis adapter into its own package
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/398#discussion_r101353985
--- Diff:
geode-redis/src/test/java/org/apache/geode/redis/AuthJUnitTest.java ---
@@ -0,0 +1,120 @@
+/*
+ * 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.redis;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.cache.GemFireCacheImpl;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.Properties;
+import redis.clients.jedis.Jedis;
+import redis.clients.jedis.exceptions.JedisConnectionException;
+import redis.clients.jedis.exceptions.JedisDataException;
+
+@Category(IntegrationTest.class)
+public class AuthJUnitTest extends RedisTestBase {
+
+ private static final String PASSWORD = "pwd";
+
+ private void setupCacheWithPassword() {
+if (cache != null) {
+ cache.close();
+}
+Properties redisCacheProperties = getDefaultRedisCacheProperties();
+
redisCacheProperties.setProperty(ConfigurationProperties.REDIS_PASSWORD,
PASSWORD);
+cache = (GemFireCacheImpl) createCacheInstance(redisCacheProperties);
+ }
+
+ @Test
+ public void testAuthConfig() {
+setupCacheWithPassword();
+InternalDistributedSystem distributedSystem =
cache.getDistributedSystem();
+assert
(distributedSystem.getConfig().getRedisPassword().equals(PASSWORD));
+cache.close();
+ }
+
+ @Test
+ public void testAuthRejectAccept() {
+setupCacheWithPassword();
+try (Jedis jedis = defaultJedisInstance()) {
+ Exception ex = null;
+ try {
+jedis.auth("wrongpwd");
+ } catch (JedisDataException e) {
+ex = e;
+ }
+ assertNotNull(ex);
+
+ String res = jedis.auth(PASSWORD);
+ assertEquals(res, "OK");
+}
+ }
+
+ @Test
+ public void testAuthNoPwd() {
+try (Jedis jedis = defaultJedisInstance()) {
+ jedis.auth(PASSWORD);
+ fail(
+ "We expecting either a JedisConnectionException or
JedisDataException to be thrown here");
--- End diff --
fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #398: Split the redis adapter into its own package
Github user galen-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/398#discussion_r101354013 --- Diff: geode-core/src/main/java/org/apache/geode/redis/GeodeRedisService.java --- @@ -0,0 +1,33 @@ +/* + * 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.redis; + +import org.apache.geode.internal.cache.CacheService; + +/** + * Created by ukohlmeyer on 2/9/17. --- End diff -- fixed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #337: [GEODE-2295] Add constructors for LocatorCancelExce...
Github user galen-pivotal closed the pull request at: https://github.com/apache/geode/pull/337 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #349: [GEODE-2324] fixes to AcceptorImpl.close()
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/349 [GEODE-2324] fixes to AcceptorImpl.close() If the thread is interrupted during closing, just continue to shut down what we can. * Catch InterruptedException so cleanup continues. * Remove top-level exception handler to avoid masking exceptions that * could short-circuit shutdown. * Fix a synchronization bug that could cause AcceptorImpl to try to shut down twice. * Fix what looks like a bug where if closing the socket throws an IOException, we fail to shut anything else down, though we still have ourselves marked as shut down. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2324 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/349.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #349 commit 26f70874102bad830f81b36d496c6469103f2e74 Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-01-18T23:00:47Z [GEODE-2324] fixes to AcceptorImpl.close() If the thread is interrupted during closing, just continue to shut down what we can. * Catch InterruptedException so cleanup continues. * Remove top-level exception handler to avoid masking exceptions that * could short-circuit shutdown. * Fix a synchronization bug that could cause AcceptorImpl to try to shut down twice. * Fix what looks like a bug where if closing the socket throws an IOException, we fail to shut anything else down, though we still have ourselves marked as shut down. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #305: [GEODE-1923] Fix a test race condition.
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/305#discussion_r91569800
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/cache/FixedPRSinglehopDUnitTest.java
---
@@ -293,18 +295,24 @@ public void test_FPAmetadataFetch() {
putIntoPartitionedRegionsThreeQs();
getFromPartitionedRegionsFor3Qs();
- Wait.pause(2000);
+ Awaitility.await().atMost(15, TimeUnit.SECONDS).until(() ->
+// Server 1 is actually primary for both Q1 and Q2, since there is
no FPA server with
--- End diff --
@bschuchardt `./gradlew spotlessApply` wants to reduce the indent on this
comment and the following lines to the same as the previous line... looks like
a bug with Spotless. I'll just move the comment above the wait.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #306: [GEODE-2191] Test randomized string serialization/d...
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/306 [GEODE-2191] Test randomized string serialization/deserialization. I added this test partly as an exercise in getting used to the Geode codebase, and partly because I'd like to see more randomized testing. The random string generator isn't perfect (I don't think all codepoints in that range are valid), but for our purposes it should be fine. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2191 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/306.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #306 commit 5cd44dd929545552ad0df1bca3219cd9d80d51cc Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2016-12-08T18:43:55Z [GEODE-2191] Test randomized string serialization/deserialization. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #306: [GEODE-2191] Test randomized string serialization/deserial...
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/306 @kirklund I disagree. We expect string serialization to work reliably on *all* inputs, and since we can't test all of them, we may as well test a bunch picked at random. If we fail, it's because of a corner case we haven't thought of. I would rather catch it randomly in testing than when a customer wants to store strings we hadn't anticipated. The seed is captured and printed so that we can reproduce the failure. There are cases where random input could show deficiencies in our test or testing environment rather than in the code under test; I don't think this is one of those. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #337: [GEODE-2295] Add constructors for LocatorCancelExce...
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/337 [GEODE-2295] Add constructors for LocatorCancelException and update one place where it's used. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2295 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/337.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #337 commit 41034c6b420b4c290012c787bfd46f0acf540030 Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-01-12T01:00:05Z [GEODE-2295] Add constructors for LocatorCancelException and update one place where it's used. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #305: [GEODE-1923] Fix a test race condition.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/305 @kohlmu-pivotal Could you merge this for me please? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #313: [GEODE-1407] Change a FlakyTest to distributedTest.
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/313#discussion_r94888520
--- Diff:
geode-core/src/test/java/org/apache/geode/cache30/ReconnectDUnitTest.java ---
@@ -534,41 +532,35 @@ public Object call() throws CacheException {
Cache cache = getCache();
Region myRegion = cache.getRegion("root/myRegion");
myRegion.put("MyKey1", "MyValue1");
-// myRegion.put("Mykey2", "MyValue2");
return savedSystem.getDistributedMember();
}
};
vm1.invoke(create1);
-
try {
-
dm = getDMID(vm0);
createGfshWaitingThread(vm0);
forceDisconnect(vm0);
newdm = waitForReconnect(vm0);
assertGfshWaitingThreadAlive(vm0);
- vm0.invoke(new SerializableRunnable("check for running locator") {
-public void run() {
- WaitCriterion wc = new WaitCriterion() {
-public boolean done() {
- return Locator.getLocator() != null;
-}
-
-public String description() {
- return "waiting for locator to restart";
-}
- };
- Wait.waitForCriterion(wc, 3, 1000, false);
+ boolean running = (Boolean) vm0.invoke(new
SerializableCallable("check for running locator") {
--- End diff --
Done, and I changed the rest of the method to be consistent.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #305: [GEODE-1923] Fix a test race condition.
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/305#discussion_r94660604
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/cache/FixedPRSinglehopDUnitTest.java
---
@@ -293,18 +294,23 @@ public void test_FPAmetadataFetch() {
putIntoPartitionedRegionsThreeQs();
getFromPartitionedRegionsFor3Qs();
- Wait.pause(2000);
+ // Server 1 is actually primary for both Q1 and Q2, since there is
no FPA server with
+ // primary set to true.
+ Awaitility.await().atMost(15, TimeUnit.SECONDS).until(
+ () -> (server1.invoke(() ->
FixedPRSinglehopDUnitTest.primaryBucketsOnServer()) == 6)
--- End diff --
Actually scratch that, I'm going to change to the style you suggested. ð
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #305: [GEODE-1923] Fix a test race condition.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/305 @kohlmu-pivotal I just think it's a bit cleaner syntactically; there's no other real benefit. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #426: GEODE-2660 Add @Experimental to the Redis adapter.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/426 @bschuchardt Check it out again; I force-pushed with just the annotation on GeodeRedisServer. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #426: GEODE-2660 Add @Experimental to the Redis adapter.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/426 @bschuchardt This is just the experimental annotation, not changes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #426: GEODE-2660 Add @Experimental to the Redis adapter.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/426 @bschuchardt want to merge this? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #426: GEODE-2660 Add @Experimental to the Redis adapter.
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/426 GEODE-2660 Add @Experimental to the Redis adapter. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/incubator-geode feature/GEODE-2660 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/426.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #426 commit 23dc29d909ab5127b8e717555bbc564bbbe1e88e Author: Galen OSullivan <gosulli...@pivotal.io> Date: 2017-03-15T17:41:42Z GEODE-2660 Add @Experimental to the Redis adapter. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #441: Fix a repetition in the README.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/441 I'm not a committer, so someone else will have to push this for me. @kohlmu-pivotal ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #404: Geode 2469
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/404 @ggreen If you can clean up the history from where my PR was merged this will be much easier to review. In general, even though we've added the `@Experimental` tag, I would like the PR to be complete and release-ready once we merge it to develop. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #437: GEODE-2653: Fix testRemoveMember and remove FlakyTest.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/437 @bschuchardt this is https://reviews.apache.org/r/58155/ --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #441: Fix a repetition in the README.
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/441 Fix a repetition in the README. and update numbers. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/geode fix-README Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/441.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #441 commit 65efe3bbba48a8be9bd427789ca833538e761ceb Author: Galen OSullivan <gosulli...@pivotal.io> Date: 2017-04-06T17:03:28Z Fix a repetition in the README. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #450: GEODE-2632: create ClientCachePutBench
Github user galen-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/450#discussion_r112004128 --- Diff: geode-core/src/jmh/java/org/apache/geode/internal/cache/tier/sockets/command/ClientCachePutBench.java --- @@ -0,0 +1,233 @@ +/* + * 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.internal.cache.tier.sockets.command; + +import static java.lang.System.*; +import static java.util.concurrent.TimeUnit.*; +import static org.apache.commons.io.FileUtils.*; +import static org.apache.commons.lang.StringUtils.*; +import static org.apache.geode.cache.client.ClientRegionShortcut.*; +import static org.apache.geode.distributed.AbstractLauncher.Status.*; +import static org.apache.geode.distributed.ConfigurationProperties.*; +import static org.apache.geode.distributed.internal.DistributionConfig.*; +import static org.apache.geode.internal.AvailablePort.*; +import static org.apache.geode.test.dunit.NetworkUtils.*; +import static org.assertj.core.api.Assertions.*; +import static org.awaitility.Awaitility.*; + +import org.apache.geode.cache.Region; +import org.apache.geode.cache.client.ClientCache; +import org.apache.geode.cache.client.ClientCacheFactory; +import org.apache.geode.distributed.ServerLauncher; +import org.apache.geode.internal.process.ProcessStreamReader; +import org.junit.rules.TemporaryFolder; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Warmup; + +import java.io.File; +import java.io.IOException; +import java.net.URL; +import java.util.ArrayList; +import java.util.List; +import java.util.Random; +import java.util.concurrent.TimeUnit; + +/** + * Benchmark that measures throughput of single-threaded client performing puts to a loner server. + * + * 100 random keys and values are generated during setup and the client reuses these in order, + * looping back around after 100 puts. + */ +@Measurement(iterations = 3, time = 2, timeUnit = MINUTES) +@Warmup(iterations = 1, time = 1, timeUnit = MINUTES) +@Fork(3) +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.SECONDS) +@State(Scope.Thread) --- End diff -- Does `ClientCachePutBench` need to be `State` when it's only got statics? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #450: GEODE-2632: create ClientCachePutBench
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/450#discussion_r112005541
--- Diff:
geode-core/src/jmh/java/org/apache/geode/internal/cache/tier/sockets/command/ClientCachePutBench.java
---
@@ -0,0 +1,233 @@
+/*
+ * 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.internal.cache.tier.sockets.command;
+
+import static java.lang.System.*;
+import static java.util.concurrent.TimeUnit.*;
+import static org.apache.commons.io.FileUtils.*;
+import static org.apache.commons.lang.StringUtils.*;
+import static org.apache.geode.cache.client.ClientRegionShortcut.*;
+import static org.apache.geode.distributed.AbstractLauncher.Status.*;
+import static org.apache.geode.distributed.ConfigurationProperties.*;
+import static org.apache.geode.distributed.internal.DistributionConfig.*;
+import static org.apache.geode.internal.AvailablePort.*;
+import static org.apache.geode.test.dunit.NetworkUtils.*;
+import static org.assertj.core.api.Assertions.*;
+import static org.awaitility.Awaitility.*;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.distributed.ServerLauncher;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.junit.rules.TemporaryFolder;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Warmup;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URL;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Benchmark that measures throughput of single-threaded client performing
puts to a loner server.
+ *
+ * 100 random keys and values are generated during setup and the client
reuses these in order,
+ * looping back around after 100 puts.
+ */
+@Measurement(iterations = 3, time = 2, timeUnit = MINUTES)
+@Warmup(iterations = 1, time = 1, timeUnit = MINUTES)
+@Fork(3)
+@BenchmarkMode(Mode.Throughput)
+@OutputTimeUnit(TimeUnit.SECONDS)
+@State(Scope.Thread)
+@SuppressWarnings("unused")
+public class ClientCachePutBench {
+
+ static final String CLASS_NAME =
ClientCachePutBench.class.getSimpleName();
+ static final String PACKAGE_NAME =
+ replace(ClientCachePutBench.class.getPackage().getName(), ".", "/");
+ static final String REGION_NAME = CLASS_NAME + "-region";
+ static final String SERVER_XML_NAME = "/" + PACKAGE_NAME + "/" +
CLASS_NAME + "-server.xml";
+ static final long PROCESS_READER_TIMEOUT = 60 * 1000;
+ static final int NUMBER_OF_KEYS = 100;
+ static final int NUMBER_OF_VALUES = 100;
+
+ @State(Scope.Benchmark)
+ public static class ClientState {
+
+Region<String, String> region;
+
+String[] keys;
+String[] values;
+
+private int keyIndex = -1;
+private int valueIndex = -1;
+
+private Process process;
+private volatile ProcessStreamReader processOutReader;
+private volatile ProcessStreamReader processErrReader;
+
+private int serverPort;
+private ServerLauncher launcher;
+private File serverDirectory;
+private ClientCache clientCache;
+
+private TemporaryFolder temporaryFolder = new Temporary
[GitHub] geode issue #450: GEODE-2632: create ClientCachePutBench
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/450 Do you know why I get so many of these messages when I run the benchmark? ``` Encountered duplicate path "javassist/bytecode/annotation/ArrayMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/BooleanMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/ByteMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/CharMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/ClassMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/DoubleMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/EnumMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/FloatMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/IntegerMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/LongMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/MemberValue.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/MemberValueVisitor.class" during copy operation configured with DuplicatesStrategy.WARN Encountered duplicate path "javassist/bytecode/annotation/ShortMemberValue.class" during copy operation configured with DuplicatesStrategy.WARN ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #450: GEODE-2632: create ClientCachePutBench
Github user galen-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/450#discussion_r112005377 --- Diff: geode-core/src/jmh/java/org/apache/geode/internal/cache/tier/sockets/command/ClientCachePutBench.java --- @@ -0,0 +1,233 @@ +/* + * 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.internal.cache.tier.sockets.command; + +import static java.lang.System.*; +import static java.util.concurrent.TimeUnit.*; +import static org.apache.commons.io.FileUtils.*; +import static org.apache.commons.lang.StringUtils.*; +import static org.apache.geode.cache.client.ClientRegionShortcut.*; +import static org.apache.geode.distributed.AbstractLauncher.Status.*; +import static org.apache.geode.distributed.ConfigurationProperties.*; +import static org.apache.geode.distributed.internal.DistributionConfig.*; +import static org.apache.geode.internal.AvailablePort.*; +import static org.apache.geode.test.dunit.NetworkUtils.*; +import static org.assertj.core.api.Assertions.*; +import static org.awaitility.Awaitility.*; + +import org.apache.geode.cache.Region; +import org.apache.geode.cache.client.ClientCache; +import org.apache.geode.cache.client.ClientCacheFactory; +import org.apache.geode.distributed.ServerLauncher; +import org.apache.geode.internal.process.ProcessStreamReader; +import org.junit.rules.TemporaryFolder; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Warmup; + +import java.io.File; +import java.io.IOException; +import java.net.URL; +import java.util.ArrayList; +import java.util.List; +import java.util.Random; +import java.util.concurrent.TimeUnit; + +/** + * Benchmark that measures throughput of single-threaded client performing puts to a loner server. + * + * 100 random keys and values are generated during setup and the client reuses these in order, + * looping back around after 100 puts. + */ +@Measurement(iterations = 3, time = 2, timeUnit = MINUTES) +@Warmup(iterations = 1, time = 1, timeUnit = MINUTES) +@Fork(3) +@BenchmarkMode(Mode.Throughput) --- End diff -- Any reason not to use `Mode.All`? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #450: GEODE-2632: create ClientCachePutBench
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/450 This will set the same keys to the same values each iteration, because. I ran a benchmark last night with a benchmark that used only 99 values, but I didn't see a significant difference: ``` Result "performPutFromClientDifferentKeys": 23532.506 ±(99.9%) 576.405 ops/s [Average] (min, avg, max) = (23176.421, 23532.506, 24073.672), stdev = 343.009 CI (99.9%): [22956.101, 24108.911] (assumes normal distribution) ``` ``` Result "performPutFromClient": 23891.204 ±(99.9%) 133.078 ops/s [Average] (min, avg, max) = (23797.325, 23891.204, 24013.180), stdev = 79.193 CI (99.9%): [23758.126, 24024.283] (assumes normal distribution) ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105059140
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SPopExecutor.java
---
@@ -36,24 +37,26 @@ public void executeCommand(Command command,
ExecutionHandlerContext context) {
}
ByteArrayWrapper key = command.getKey();
-@SuppressWarnings("unchecked")
-Region<ByteArrayWrapper, Boolean> keyRegion =
-(Region<ByteArrayWrapper, Boolean>)
context.getRegionProvider().getRegion(key);
-if (keyRegion == null || keyRegion.isEmpty()) {
+// getRegion(key);
+Region<ByteArrayWrapper, Set> region =
getRegion(context);
--- End diff --
We still need to check the metadata region to determine whether to throw
WRONGTYPE or return nil.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105061296
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SetOpExecutor.java
---
@@ -47,23 +44,23 @@ public void executeCommand(Command command,
ExecutionHandlerContext context) {
destination = command.getKey();
ByteArrayWrapper firstSetKey = new
ByteArrayWrapper(commandElems.get(setsStartIndex++));
-if (!isStorage())
- checkDataType(firstSetKey, RedisDataType.REDIS_SET, context);
-Region<ByteArrayWrapper, Boolean> region =
-(Region<ByteArrayWrapper, Boolean>) rC.getRegion(firstSetKey);
-Set firstSet = null;
-if (region != null) {
- firstSet = new HashSet(region.keySet());
-}
-ArrayList<Set> setList = new
ArrayList<Set>();
+// if (!isStorage())
--- End diff --
This is gnarly. I'll have to look at this later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105008854
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/ExecutionHandlerContext.java
---
@@ -135,8 +135,14 @@ private void writeToChannel(ByteBuf message) {
*/
@Override
public void channelRead(ChannelHandlerContext ctx, Object msg) throws
Exception {
-Command command = (Command) msg;
-executeCommand(ctx, command);
+try {
+ Command command = (Command) msg;
+ executeCommand(ctx, command);
+} catch (Exception e) {
+ logger.error(e);
--- End diff --
If we're going to be logging exceptions as they propagate, I'd like to
think about the best place to catch all of them and also it would be nice to
add a little error message so we know where we caught the exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105060019
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SRandMemberExecutor.java
---
@@ -42,9 +42,8 @@ public void executeCommand(Command command,
ExecutionHandlerContext context) {
}
ByteArrayWrapper key = command.getKey();
-@SuppressWarnings("unchecked")
-Region<ByteArrayWrapper, Boolean> keyRegion =
-(Region<ByteArrayWrapper, Boolean>)
context.getRegionProvider().getRegion(key);
+
--- End diff --
I think we still need to check the metadata region here again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105046939
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SMembersExecutor.java
---
@@ -39,16 +39,18 @@ public void executeCommand(Command command,
ExecutionHandlerContext context) {
ByteArrayWrapper key = command.getKey();
checkDataType(key, RedisDataType.REDIS_SET, context);
-@SuppressWarnings("unchecked")
-Region<ByteArrayWrapper, Boolean> keyRegion =
-(Region<ByteArrayWrapper, Boolean>)
context.getRegionProvider().getRegion(key);
-if (keyRegion == null) {
+Region<ByteArrayWrapper, Set> region =
getRegion(context);
+
+// companies:ea64fe8c-e0a0-4439-a05d-d0738dd5ef80:idx
--- End diff --
What does this comment mean?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105048647
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SIsMemberExecutor.java
---
@@ -40,23 +42,35 @@ public void executeCommand(Command command,
ExecutionHandlerContext context) {
return;
}
+RegionProvider regionProvider = context.getRegionProvider();
+
+// check by meta data key
+// SISMEMBER companies ea64fe8c-e0a0-4439-a05d-d0738dd5ef80
ByteArrayWrapper key = command.getKey();
+if (!regionProvider.existsKey(key)) {
+
command.setResponse(Coder.getIntegerResponse(context.getByteBufAllocator(),
NOT_EXISTS));
+ return;
+}
+
--- End diff --
We should check the data type here, as Redis gives a different error for a
key that is of the wrong datatype than it does for a nonexistent key.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105008897
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/ExecutionHandlerContext.java
---
@@ -235,6 +242,8 @@ private void executeWithoutTransaction(final Executor
exec, Command command) thr
exec.executeCommand(command, this);
return;
} catch (Exception e) {
+logger.error(e);
--- End diff --
see error logging comment above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105060205
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SRandMemberExecutor.java
---
@@ -42,9 +42,8 @@ public void executeCommand(Command command,
ExecutionHandlerContext context) {
}
ByteArrayWrapper key = command.getKey();
-@SuppressWarnings("unchecked")
-Region<ByteArrayWrapper, Boolean> keyRegion =
-(Region<ByteArrayWrapper, Boolean>)
context.getRegionProvider().getRegion(key);
+
--- End diff --
Perhaps we should make getRegion check the Metadata region and throw a
checked TypeException if the type is wrong?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105010443
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java ---
@@ -187,8 +215,18 @@ public boolean removeKey(ByteArrayWrapper key,
RedisDataType type, boolean cance
return this.stringsRegion.remove(key) != null;
} else if (type == RedisDataType.REDIS_HLL) {
return this.hLLRegion.remove(key) != null;
+} else if (type == RedisDataType.REDIS_LIST) {
+ return this.destroyRegion(key, type);
+} else if (type == RedisDataType.REDIS_SET) {
+ // remove the set
--- End diff --
I think the comment is redundant and the returns aren't necessary (&
distract from overall control flow).
A comment saying "sets & hashes get removed from the region they're put in
rather than destroying a region" would be fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105053042
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SMoveExecutor.java
---
@@ -45,30 +48,44 @@ public void executeCommand(Command command,
ExecutionHandlerContext context) {
checkDataType(source, RedisDataType.REDIS_SET, context);
checkDataType(destination, RedisDataType.REDIS_SET, context);
-@SuppressWarnings("unchecked")
-Region<ByteArrayWrapper, Boolean> sourceRegion =
-(Region<ByteArrayWrapper, Boolean>)
context.getRegionProvider().getRegion(source);
-if (sourceRegion == null) {
+Region<ByteArrayWrapper, Set> region =
getRegion(context);
+
+Set sourceSet = region.get(source);
+
+if (sourceSet == null) {
command.setResponse(Coder.getIntegerResponse(context.getByteBufAllocator(),
NOT_MOVED));
return;
}
-Object oldVal = sourceRegion.get(mem);
-sourceRegion.remove(mem);
+sourceSet = new HashSet(sourceSet); // copy to
support transactions;
--- End diff --
Copying helps with concurrent modifications on the same machine, but we can
still lose set adds 'cause race conditions (A reads, B reads, A adds key "foo"
to a copy, B adds key "bar" to a copy, A puts, B puts, the key added by A is
lost). I think the `ExecutionHandlerContext` provides transaction
support... txn support in `ExecutionHandlerContext` looks like it's only
for MULTI/EXEC.
I think we can do transactions in Geode by locking a particular key, or two
keys if they're on the same server. However, if they aren't then we'll just
have to say "whoops!" and return an error (Redis cluster does the same thing).
Maybe just mark it as a FIXME for now, and carry on?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105009858
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java ---
@@ -95,18 +98,38 @@ public RegionProvider(Region<ByteArrayWrapper,
ByteArrayWrapper> stringsRegion,
Region<ByteArrayWrapper, HyperLogLogPlus> hLLRegion,
Region<String, RedisDataType> redisMetaRegion,
ConcurrentMap<ByteArrayWrapper, ScheduledFuture> expirationsMap,
- ScheduledExecutorService expirationExecutor, RegionShortcut
defaultShortcut) {
+ ScheduledExecutorService expirationExecutor, RegionShortcut
defaultShortcut,
+ Region<ByteArrayWrapper, Map<ByteArrayWrapper, ByteArrayWrapper>>
hashRegion,
+ Region<ByteArrayWrapper, Set> setRegion) {
+
+this(stringsRegion, hLLRegion, redisMetaRegion, expirationsMap,
expirationExecutor,
+defaultShortcut, hashRegion, setRegion,
GemFireCacheImpl.getInstance());
--- End diff --
ð for making the cache an argument to the constructor (even if it's
optional). Anything that makes things a little more decoupled.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r104552749
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/hash/HashExecutor.java
---
@@ -75,14 +96,76 @@
}
/**
+ * Return key from format region:key
+ *
+ * @param key the raw key
+ * @return the ByteArray for the key
+ */
+ public static ByteArrayWrapper toEntryKey(ByteArrayWrapper key) {
--- End diff --
Can this be private?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105010837
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java ---
@@ -289,7 +331,9 @@ public void
createRemoteRegionReferenceLocally(ByteArrayWrapper key, RedisDataTy
Exception concurrentCreateDestroyException = null;
do {
concurrentCreateDestroyException = null;
- r = createRegionGlobally(stringKey);
+
+ r = createRegionGlobally(regionName);
--- End diff --
Why change `stringKey` to `regionName`, another variable with the same
value?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #412: GEODE-1793 LocatorDUnitTest.testStartTwoLocatorsOne...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/412#discussion_r104258182
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
---
@@ -360,6 +360,13 @@ private void processRequest(final Socket sock) {
versionOrdinal = (short)
GOSSIP_TO_GEMFIRE_VERSION_MAP.get(gossipVersion);
} else {
// Close the socket. We can not accept requests from a newer
version
+ try {
+sock.getOutputStream().write("unknown protocol
version".getBytes());
--- End diff --
Is there any risk of this being interpreted by anything other than garbage
on the other side?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r105510564
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/hash/HSetExecutor.java
---
@@ -55,9 +69,11 @@ public void executeCommand(Command command,
ExecutionHandlerContext context) {
Object oldValue;
if (onlySetOnAbsent())
- oldValue = keyRegion.putIfAbsent(field, new ByteArrayWrapper(value));
+ oldValue = map.putIfAbsent(field, new ByteArrayWrapper(value));
--- End diff --
This will fail with `ConcurrentModificationException` if other connections
try to access the same hash at the same time. I think we need to clone and then
put, which will give us eventual consistency but not ACID.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r104041093
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/hash/HDelExecutor.java
---
@@ -40,25 +38,25 @@ public void executeCommand(Command command,
ExecutionHandlerContext context) {
int numDeleted = 0;
ByteArrayWrapper key = command.getKey();
+Map<ByteArrayWrapper, ByteArrayWrapper> map = getMap(context, key);
-checkDataType(key, RedisDataType.REDIS_HASH, context);
-Region<ByteArrayWrapper, ByteArrayWrapper> keyRegion =
getRegion(context, key);
-
-if (keyRegion == null) {
+if (map == null || map.isEmpty()) {
command.setResponse(Coder.getIntegerResponse(context.getByteBufAllocator(),
numDeleted));
return;
}
-
for (int i = START_FIELDS_INDEX; i < commandElems.size(); i++) {
ByteArrayWrapper field = new ByteArrayWrapper(commandElems.get(i));
- Object oldValue = keyRegion.remove(field);
+ Object oldValue = map.remove(field);
if (oldValue != null)
numDeleted++;
}
-if (keyRegion.isEmpty()) {
- context.getRegionProvider().removeKey(key, RedisDataType.REDIS_HASH);
-}
+// save map
+saveMap(map, context, key);
+
+// if (map.isEmpty()) {
--- End diff --
Would prefer to remove code rather than comment it out.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r104031973
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java ---
@@ -442,9 +502,10 @@ protected void checkDataType(ByteArrayWrapper key,
RedisDataType type) {
return;
if (currentType == RedisDataType.REDIS_PROTECTED)
throw new RedisDataTypeMismatchException("The key name \"" + key +
"\" is protected");
-if (currentType != type)
--- End diff --
Why has this been removed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r104041266
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java ---
@@ -442,9 +502,10 @@ protected void checkDataType(ByteArrayWrapper key,
RedisDataType type) {
return;
if (currentType == RedisDataType.REDIS_PROTECTED)
throw new RedisDataTypeMismatchException("The key name \"" + key +
"\" is protected");
-if (currentType != type)
- throw new RedisDataTypeMismatchException(
- "The key name \"" + key + "\" is already used by a " +
currentType.toString());
+/*
--- End diff --
Would prefer to remove code rather than comment it out.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #404: Geode 2469
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/404#discussion_r104059948
--- Diff:
geode-core/src/main/java/org/apache/geode/redis/internal/executor/hash/HashInterpreter.java
---
@@ -0,0 +1,126 @@
+/*
+ * 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.redis.internal.executor.hash;
+
+import java.util.Map;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.GeodeRedisServer;
+import org.apache.geode.redis.internal.ByteArrayWrapper;
+import org.apache.geode.redis.internal.Coder;
+import org.apache.geode.redis.internal.ExecutionHandlerContext;
+import org.apache.geode.redis.internal.RedisDataType;
+
+/**
+ * Utility class for interpreting and processing Redis Hash data structure
+ *
+ *
+ */
+public class HashInterpreter {
+
+ /**
+ *
+ * The region:key separator.
+ *
+ * REGION_KEY_SEPERATOR = ":"
+ *
+ * See Hash section of https://redis.io/topics/data-types;>https://redis.io/topics/data-types#Hashes
+ *
+ */
+ public static final String REGION_KEY_SEPERATOR = ":";
+
+ /**
+ * The default hash region name REGION_HASH_REGION =
Coder.stringToByteArrayWrapper("ReDiS_HASH")
+ */
+ public static final ByteArrayWrapper REGION_HASH_REGION =
+ Coder.stringToByteArrayWrapper(GeodeRedisServer.HASH_REGION);
+
+ /**
+ * Return the region presenting the hash
+ *
+ * @param key the raw Redis command key that may contain the region:key
formation
+ * @param context the exception handler context
+ * @return the region were the command's data will be processed
+ */
+ @SuppressWarnings("unchecked")
+ public static Region<ByteArrayWrapper, Map<ByteArrayWrapper,
ByteArrayWrapper>> getRegion(
--- End diff --
I think the abstraction would be cleaner if the HashInterpreter
implemented `setHash` and `getHash` (and `setHashKey` and `getHashKey`)
methods, and callers didn't have to think about regions at all. It would also
make it clearer where the single point of concern is for where and how hashes
get stored.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #437: GEODE-2653: Fix testRemoveMember and remove FlakyTest.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/437 @kohlmu-pivotal --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #663: GEODE-3314: Fix DLockService token leak.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/663 @pivotal-amurmann I'd like to write a unit test. @hiteshk25 and I took a stab at it this afternoon but after an hour or two, got lost in mocks and the tangle of DistributedSystem/DistributedMember/DistributionManager/Elder/RequestProcessor . I would love to do some refactoring to make this logic more testable, but I'd also like to see this fix get into master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #676: GEODE-3321: Adding ErrorCode values to protobuf protocol
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/676 Looks like there are some conflicts that need to be resolved. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #676: GEODE-3321: Adding ErrorCode values to protobuf pro...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/676#discussion_r130987753
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtocolErrorCode.java
---
@@ -0,0 +1,39 @@
+/*
+ * 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/LICENSE2.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.protocol.protobuf;
+
+public enum ProtocolErrorCode {
+ GENERIC_FAILURE(1000),
--- End diff --
sure, just curious.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #661: GEODE-3319 - refactor to use protobuf encoding for ...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/661#discussion_r130494791
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufPrimitiveTypes.java
---
@@ -0,0 +1,47 @@
+/*
+ * 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.protocol.protobuf.utilities;
+
+import
org.apache.geode.protocol.protobuf.utilities.exception.UnknownProtobufPrimitiveType;
+
+public enum ProtobufPrimitiveTypes {
+
+ STRING(String.class),
+ INT(Integer.class),
+ LONG(Long.class),
+ SHORT(Short.class),
+ BYTE(Byte.class),
+ BOOLEAN(Boolean.class),
+ DOUBLE(Double.class),
+ FLOAT(Float.class),
+ BINARY(byte[].class);
+
+ private Class clazzType;
--- End diff --
I think `clazz` would be clearer than `clazzType`. What is a type of class?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #661: GEODE-3319 - refactor to use protobuf encoding for ...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/661#discussion_r130779627
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java
---
@@ -50,10 +50,7 @@
return Success.of(RegionAPI.PutResponse.newBuilder().build());
} catch (ClassCastException ex) {
return Failure.of(BasicTypes.ErrorResponse.newBuilder()
-.setMessage("invalid key or value type for region " +
regionName + ",passed key: "
--- End diff --
I'm curious why you chose to shrink the message here; what am I missing?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #661: GEODE-3319 - refactor to use protobuf encoding for ...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/661#discussion_r130495450
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufPrimitiveTypes.java
---
@@ -0,0 +1,47 @@
+/*
+ * 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.protocol.protobuf.utilities;
+
+import
org.apache.geode.protocol.protobuf.utilities.exception.UnknownProtobufPrimitiveType;
+
+public enum ProtobufPrimitiveTypes {
+
+ STRING(String.class),
+ INT(Integer.class),
+ LONG(Long.class),
+ SHORT(Short.class),
+ BYTE(Byte.class),
+ BOOLEAN(Boolean.class),
+ DOUBLE(Double.class),
+ FLOAT(Float.class),
+ BINARY(byte[].class);
+
+ private Class clazzType;
+
+ ProtobufPrimitiveTypes(Class clazz) {
+this.clazzType = clazz;
+ }
+
+ public static ProtobufPrimitiveTypes valueOf(Class unencodedValueClass)
+ throws UnknownProtobufPrimitiveType {
+for (ProtobufPrimitiveTypes protobufPrimitiveTypes : values()) {
--- End diff --
I think the variable here should be singular as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #661: GEODE-3319 - refactor to use protobuf encoding for ...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/661#discussion_r130494367
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufPrimitiveTypes.java
---
@@ -0,0 +1,47 @@
+/*
+ * 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.protocol.protobuf.utilities;
+
+import
org.apache.geode.protocol.protobuf.utilities.exception.UnknownProtobufPrimitiveType;
+
+public enum ProtobufPrimitiveTypes {
--- End diff --
I think it's better to use the singular `ProtobufPrimitiveType` here, since
each represents a protobuf primitive type.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #676: GEODE-3321: Adding ErrorCode values to protobuf protocol
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/676 It looks like this is failing in Travis due to `:rat`, but you've got a license in your only new file. I'm a bit mystified. Closing the pull request and reopening it can be a good way to trigger a new Travis build. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #676: GEODE-3321: Adding ErrorCode values to protobuf pro...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/676#discussion_r130781170
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtocolErrorCode.java
---
@@ -0,0 +1,39 @@
+/*
+ * 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/LICENSE2.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.protocol.protobuf;
+
+public enum ProtocolErrorCode {
+ GENERIC_FAILURE(1000),
--- End diff --
Why are we starting at 1000?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/673#discussion_r130781962
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
---
@@ -0,0 +1,110 @@
+/*
+ * 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.protocol.protobuf.operations;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.client.internal.locator.GetAllServersRequest;
+import
org.apache.geode.cache.client.internal.locator.GetAllServersResponse;
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.tcpserver.TcpClient;
+import org.apache.geode.internal.admin.remote.DistributionLocatorId;
+import org.apache.geode.protocol.operations.OperationHandler;
+import org.apache.geode.protocol.protobuf.BasicTypes;
+import org.apache.geode.protocol.protobuf.Failure;
+import org.apache.geode.protocol.protobuf.Result;
+import org.apache.geode.protocol.protobuf.ServerAPI;
+import org.apache.geode.protocol.protobuf.Success;
+import org.apache.geode.serialization.SerializationService;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+public class GetAvailableServersOperationHandler implements
+OperationHandler<ServerAPI.GetAvailableServersRequest,
ServerAPI.GetAvailableServersResponse> {
+
+ @Override
+ public Result process(
+ SerializationService serializationService,
ServerAPI.GetAvailableServersRequest request,
+ Cache cache) {
+HashSet locators =
getLocatorIdsFromDistributedProperties(cache);
+
+TcpClient tcpClient = getTcpClient();
+Optional<Result> result =
locators.stream()
+.map((locatorId -> getGetAvailableServersFromLocator(tcpClient,
locatorId)))
+.filter((obj) -> obj != null).findFirst();
+
+if (result.isPresent()) {
+ return result.get();
+} else {
+ return Failure
+ .of(BasicTypes.ErrorResponse.newBuilder().setMessage("Unable to
find a locator").build());
+}
+ }
+
+ private HashSet
getLocatorIdsFromDistributedProperties(Cache cache) {
--- End diff --
Will this work if locators are configured via cluster config? I'm guessing
it won't work if the original locators are no longer in the cluster. Since this
is a prototype and will be redone better, I think it's fine for now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers
Github user galen-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/673#discussion_r130782011 --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java --- @@ -0,0 +1,110 @@ +/* + * 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.protocol.protobuf.operations; + +import org.apache.commons.lang.StringUtils; +import org.apache.geode.cache.Cache; +import org.apache.geode.cache.client.internal.locator.GetAllServersRequest; +import org.apache.geode.cache.client.internal.locator.GetAllServersResponse; +import org.apache.geode.distributed.ConfigurationProperties; +import org.apache.geode.distributed.internal.InternalDistributedSystem; +import org.apache.geode.distributed.internal.ServerLocation; +import org.apache.geode.distributed.internal.tcpserver.TcpClient; +import org.apache.geode.internal.admin.remote.DistributionLocatorId; +import org.apache.geode.protocol.operations.OperationHandler; +import org.apache.geode.protocol.protobuf.BasicTypes; +import org.apache.geode.protocol.protobuf.Failure; +import org.apache.geode.protocol.protobuf.Result; +import org.apache.geode.protocol.protobuf.ServerAPI; +import org.apache.geode.protocol.protobuf.Success; +import org.apache.geode.serialization.SerializationService; + +import java.io.IOException; +import java.net.InetSocketAddress; +import java.util.Collection; +import java.util.HashSet; +import java.util.Optional; +import java.util.Properties; +import java.util.StringTokenizer; +import java.util.stream.Collectors; + +public class GetAvailableServersOperationHandler implements --- End diff -- @Experimental or a doc comment explaining why this won't be around forever would be awesome. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #676: GEODE-3321: Adding ErrorCode values to protobuf pro...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/676#discussion_r130781436
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandlerJUnitTest.java
---
@@ -95,8 +97,8 @@ public void
processReturnsUnsucessfulResponseForInvalidRegion()
operationHandler.process(serializationServiceStub, removeRequest,
cacheStub);
assertTrue(result instanceof Failure);
-org.junit.Assert.assertThat(result.getErrorMessage().getMessage(),
-CoreMatchers.containsString("Region"));
+assertEquals(ProtocolErrorCode.REGION_NOT_FOUND.codeValue,
--- End diff --
YES. I like these kinds of tests much better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/683#discussion_r131290606
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockToken.java
---
@@ -302,14 +302,14 @@ boolean checkForExpiration() {
* @param remoteThread identity of the leasing thread
* @return true if lease for this lock token is successfully granted
*/
- synchronized boolean grantLock(long newLeaseExpireTime, int newLeaseId,
int newRecursion,
+ synchronized void grantLock(long newLeaseExpireTime, int newLeaseId, int
newRecursion,
RemoteThread remoteThread) {
Assert.assertTrue(remoteThread != null);
Assert.assertTrue(newLeaseId > -1, "Invalid attempt to grant lock with
leaseId " + newLeaseId);
checkDestroyed();
-checkForExpiration();
+checkForExpiration(); // TODO: this should throw.
--- End diff --
I may have misunderstood the intent of calling this function when I added
the comment. It could use a second look.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...
GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/683 GEODE-3314 - additional refactoring for developer QoL. * Write characterization tests for DLockService. * Remove debugging code. * Remove dead code. * Remove comments. * Extract the local lock granting into a separate function. Between the characterization tests we've written and the existing DUnit tests, the coverage should be fairly adequate. Signed-off-by: Hitesh Khamesra <hkames...@pivotal.io> Signed-off-by: Galen O'Sullivan <gosulli...@pivotal.io> 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? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x] Is your initial contribution a single, squashed commit? - [ ] 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 dev@geode.apache.org. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/geode feature/GEODE-3314-2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/683.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #683 commit 6ec19370da30004347b20c22563268f6adfd35c1 Author: Galen O'Sullivan <gosulli...@pivotal.io> Date: 2017-08-02T18:29:21Z GEODE-3314 - additional refactoring for developer QoL. * Write characterization tests for DLockService. * Remove debugging code. * Remove dead code. * Remove comments. * Extract the local lock granting into a separate function. Between the characterization tests we've written and the existing DUnit tests, the coverage should be fairly adequate. Signed-off-by: Hitesh Khamesra <hkames...@pivotal.io> Signed-off-by: Galen O'Sullivan <gosulli...@pivotal.io> --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #683: GEODE-3314 - additional refactoring for developer QoL.
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/683 @kohlmu-pivotal @hiteshk25 @bschuchardt @pivotal-amurmann @WireBaron --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/683#discussion_r131290142
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockRequestProcessor.java
---
@@ -196,6 +196,13 @@ long getLeaseExpireTime() {
return this.response.leaseExpireTime;
}
+ /**
+ *
+ * @param interruptible
+ * @param lockId
+ * @return
+ * @throws InterruptedException only possible if interruptible is true.
+ */
protected boolean requestLock(boolean interruptible, int lockId) throws
InterruptedException {
final boolean isDebugEnabled_DLS =
logger.isTraceEnabled(LogMarker.DLS);
--- End diff --
The `LogMarker` is used in the `logger.trace` function. This is based on an
old pattern that was written because an old version of the logger was slow, so
it was better to gate on a boolean for performance. The logging function will
do an identical check to this one before printing a log message.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/683#discussion_r131290383
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockToken.java
---
@@ -302,14 +302,14 @@ boolean checkForExpiration() {
* @param remoteThread identity of the leasing thread
* @return true if lease for this lock token is successfully granted
*/
- synchronized boolean grantLock(long newLeaseExpireTime, int newLeaseId,
int newRecursion,
+ synchronized void grantLock(long newLeaseExpireTime, int newLeaseId, int
newRecursion,
RemoteThread remoteThread) {
Assert.assertTrue(remoteThread != null);
Assert.assertTrue(newLeaseId > -1, "Invalid attempt to grant lock with
leaseId " + newLeaseId);
checkDestroyed();
-checkForExpiration();
+checkForExpiration(); // TODO: this should throw.
--- End diff --
yeah, that's probably a good idea. It looks like `checkForExpiration` is
being used for the side effect of expiring the lock if it's run out of time.
This method could be made clearer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...
Github user galen-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/683#discussion_r131290034 --- Diff: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockToken.java --- @@ -87,7 +87,8 @@ private Thread thread; /** - * Number of threads currently using this lock token. + * Number of usages of this lock token. usageCount = recursion + (# of threads waiting for this + * lock). It's weird, I know. --- End diff -- I could go either way. I don't mind a bit of humour and dialogue in our codebase. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #698: Mark ProtoBuf interface as experimental
Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/698 What you've done looks good. However, it looks like there are ten or so files in `geode-protobuf` that you didn't mark as experimental. Is there a reason for this? Have a look at the difference between these: ```bash find geode-protobuf/src/main/ -name '*.java' find geode-protobuf/src/main/ -name '*.java' | xargs grep -l '@Experimental' ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #682: GEODE-3393: One-way SSL authentication fails with F...
Github user galen-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/682#discussion_r132626571 --- Diff: geode-core/src/main/java/org/apache/geode/internal/admin/SSLConfig.java --- @@ -33,11 +34,11 @@ private String ciphers = DistributionConfig.DEFAULT_SSL_CIPHERS; private boolean requireAuth = DistributionConfig.DEFAULT_SSL_REQUIRE_AUTHENTICATION; private String keystore = DistributionConfig.DEFAULT_SSL_KEYSTORE; - private String keystoreType = DistributionConfig.DEFAULT_CLUSTER_SSL_KEYSTORE_TYPE; + private String keystoreType = KeyStore.getDefaultType(); --- End diff -- I notice that `DistributionConfig.DEFAULT_CLUSTER_SSL_KEYSTORE_TYPE` is marked as deprecated. Is this its replacement? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #683: GEODE-3314 - additional refactoring for developer Q...
Github user galen-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/683#discussion_r132864196 --- Diff: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java --- @@ -74,17 +73,6 @@ public static final long NOT_GRANTOR_SLEEP = Long .getLong(DistributionConfig.GEMFIRE_PREFIX + "DLockService.notGrantorSleep", 100).longValue(); - public static final boolean DEBUG_DISALLOW_NOT_HOLDER = Boolean - .getBoolean(DistributionConfig.GEMFIRE_PREFIX + "DLockService.debug.disallowNotHolder"); - - public static final boolean DEBUG_LOCK_REQUEST_LOOP = Boolean - .getBoolean(DistributionConfig.GEMFIRE_PREFIX + "DLockService.debug.disallowLockRequestLoop"); - - public static final int DEBUG_LOCK_REQUEST_LOOP_COUNT = Integer - .getInteger( - DistributionConfig.GEMFIRE_PREFIX + "DLockService.debug.disallowLockRequestLoopCount", 20) - .intValue(); - public static final boolean DEBUG_NONGRANTOR_DESTROY_LOOP = Boolean --- End diff -- These are used in `basicDestroy`, which is separate from `tryInterruptibly`, the method we're refactoring here. I'd rather keep the changeset relatively small and isolated for now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/707#discussion_r132878378
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
---
@@ -63,6 +65,31 @@ private static ClientProtocolMessageHandler
findClientProtocolMessageHandler() {
}
}
+ private static Class
findStreamAuthenticator(
+ String implementationID) {
+if (authenticatorClass != null) {
+ return authenticatorClass;
+}
+
+synchronized (streamAuthenticatorLoadLock) {
--- End diff --
Once `authenticatorClass` is initialized, there will no longer be any
synchronization necessary in this method. This means that each thread will need
to synchronize at most once, which means no locking / cache misses.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/707#discussion_r132878512
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
---
@@ -72,9 +99,15 @@ public static ServerConnection
makeServerConnection(Socket s, InternalCache c,
throw new IOException("Acceptor received unknown communication
mode: " + communicationMode);
} else {
protobufProtocolHandler = findClientProtocolMessageHandler();
-return new GenericProtocolServerConnection(s, c, helper, stats,
hsTimeout, socketBufferSize,
-communicationModeStr, communicationMode, acceptor,
protobufProtocolHandler,
-securityService);
+authenticatorClass = findStreamAuthenticator(
+
c.getInternalDistributedSystem().getConfig().getProtobufProtocolAuthenticationMode());
+try {
+ return new GenericProtocolServerConnection(s, c, helper, stats,
hsTimeout,
+ socketBufferSize, communicationModeStr, communicationMode,
acceptor,
+ protobufProtocolHandler, securityService,
authenticatorClass.newInstance());
--- End diff --
We find the class exactly once, and create a new instance per protocol
instance. We then create an instance per `GenericProtocolServerConnection`.
This is because the authenticator (unlike the protocol handler) is stateful, so
we can't share it between connections.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
