[GitHub] geode pull request #611: GEODE-3145 Add new protocol to Geode JAR
GitHub user pivotal-amurmann opened a pull request: https://github.com/apache/geode/pull/611 GEODE-3145 Add new protocol to Geode JAR 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? GEODE-3145 - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x] Is your initial contribution a single, squashed commit? - [x] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [x] 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/pivotal-amurmann/geode feature/GEODE-3145 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/611.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 #611 commit 3c19ea2a5ac3d5265b94aa55c453acadb198e340 Author: Alexander Murmann Date: 2017-06-28T20:59:43Z GEODE-3145 Add new protocol to Geode JAR --- 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 #611: GEODE-3145 Add new protocol to Geode JAR
Github user pivotal-amurmann closed the pull request at: https://github.com/apache/geode/pull/611 --- 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 #624: GEODE-2998: Add remove operation
GitHub user pivotal-amurmann opened a pull request: https://github.com/apache/geode/pull/624 GEODE-2998: Add remove operation 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? GEODE-2998 - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x Is your initial contribution a single, squashed commit? - [x] Does `gradlew build` run cleanly? - [x] Have you written or updated unit tests to verify your changes? - [x] 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)? does not apply ### 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/pivotal-amurmann/geode GEODE-2998 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/624.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 #624 commit bd1844d95d106d13328756dfeaea5f7731e0a089 Author: Brian Rowe Date: 2017-07-06T23:36:51Z GEODE-2998: Add remove operation Signed-off-by: Alexander Murmann Signed-off-by: Matt Kalinowski --- 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 #630: GEODE-3141: GetRegion Operation implemented
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/630#discussion_r127285154
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
---
@@ -278,6 +278,31 @@ public void
useSSL_testNewProtocolHeaderLeadsToNewProtocolServerConnection() thr
testNewProtocolHeaderLeadsToNewProtocolServerConnection();
}
+ @Test
+ public void testNewProtocolGetRegionCallSucceeds() throws Exception {
+System.setProperty("geode.feature-protobuf-protocol", "true");
+
+Socket socket = new Socket("localhost", cacheServerPort);
+Awaitility.await().atMost(5,
TimeUnit.SECONDS).until(socket::isConnected);
+OutputStream outputStream = socket.getOutputStream();
+outputStream.write(110);
+
+
+ProtobufProtocolSerializer protobufProtocolSerializer = new
ProtobufProtocolSerializer();
+ClientProtocol.Message getRegionMessage =
+MessageUtil.makeGetRegionRequestMessage(TEST_REGION,
ClientProtocol.MessageHeader.newBuilder().build());
+protobufProtocolSerializer.serialize(getRegionMessage, outputStream);
+
+ClientProtocol.Message message =
+protobufProtocolSerializer.deserialize(socket.getInputStream());
+assertEquals(ClientProtocol.Message.MessageTypeCase.RESPONSE,
message.getMessageTypeCase());
+ClientProtocol.Response response = message.getResponse();
+assertEquals(ClientProtocol.Response.ResponseAPICase.GETREGIONRESPONSE,
+response.getResponseAPICase());
+response.getGetRegionResponse();
+//TODO add some assertions for Region data
--- End diff --
Can we either add the assertions or put a chore in the backlog to backfill
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 issue #634: Feature/geode 3175
Github user pivotal-amurmann commented on the issue: https://github.com/apache/geode/pull/634 Should this be squashed? --- 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 #646: GEODE-3213: Refactor ProtoBuf handler flow
GitHub user pivotal-amurmann opened a pull request: https://github.com/apache/geode/pull/646 GEODE-3213: Refactor ProtoBuf handler flow Signed-off-by: Alexander Murmann 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? GEODE-3213 - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x] Is your initial contribution a single, squashed commit? - [x] Does `gradlew build` run cleanly? - [x] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? NA ### 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/pivotal-amurmann/geode GEODE-3213 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/646.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 #646 commit bc9de5361e288199fbd0f22264ebf2a6ba6190e2 Author: Galen O'Sullivan Date: 2017-07-17T18:18:57Z GEODE-3213: Refactor ProtoBuf handler flow Signed-off-by: Alexander Murmann --- 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 #646: GEODE-3213: Refactor ProtoBuf handler flow
Github user pivotal-amurmann commented on the issue: https://github.com/apache/geode/pull/646 @kohlmu-pivotal @WireBaron @hiteshk25 @galen-pivotal @bschuchardt --- 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 #646: GEODE-3213: Refactor ProtoBuf handler flow
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/646#discussion_r128618059
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
---
@@ -15,32 +15,35 @@
package org.apache.geode.protocol.protobuf;
import org.apache.geode.cache.Cache;
-import org.apache.geode.protocol.exception.InvalidProtocolMessageException;
-import org.apache.geode.protocol.operations.OperationHandler;
-import
org.apache.geode.protocol.operations.registry.OperationsHandlerRegistry;
-import
org.apache.geode.protocol.operations.registry.exception.OperationHandlerNotRegisteredException;
+import org.apache.geode.protocol.operations.OperationContext;
+import org.apache.geode.protocol.operations.Result;
+import
org.apache.geode.protocol.operations.registry.OperationContextRegistry;
import org.apache.geode.serialization.SerializationService;
/**
* This handles protobuf requests by determining the operation type of the
request and dispatching
* it to the appropriate handler.
*/
public class ProtobufOpsProcessor {
- private final OperationsHandlerRegistry opsHandlerRegistry;
+
+ private final OperationContextRegistry operationContextRegistry;
private final SerializationService serializationService;
- public ProtobufOpsProcessor(OperationsHandlerRegistry opsHandlerRegistry,
- SerializationService serializationService) {
-this.opsHandlerRegistry = opsHandlerRegistry;
+ public ProtobufOpsProcessor(SerializationService serializationService,
+ OperationContextRegistry operationsContextRegistry) {
this.serializationService = serializationService;
+this.operationContextRegistry = operationsContextRegistry;
}
- public ClientProtocol.Response process(ClientProtocol.Request request,
Cache cache)
- throws OperationHandlerNotRegisteredException,
InvalidProtocolMessageException {
+ public ClientProtocol.Response process(ClientProtocol.Request request,
Cache cache) {
ClientProtocol.Request.RequestAPICase requestType =
request.getRequestAPICase();
-OperationHandler opsHandler =
-
opsHandlerRegistry.getOperationHandlerForOperationId(requestType.getNumber());
+OperationContext operationContext =
operationContextRegistry.getOperationContext(requestType);
--- End diff --
Currently we are getting a NPE for those which is awful and already lead to
wasted time debugging the non-obvious error. Let's go back to throwing
something specific.
---
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 #646: GEODE-3213: Refactor ProtoBuf handler flow
Github user pivotal-amurmann commented on a diff in the pull request: https://github.com/apache/geode/pull/646#discussion_r128648851 --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/operations/Failure.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.operations; --- End diff -- We wrote a test that enforces that every enum value has a handler. Of course that test won't pass until we have implemented all operations which isn't going to happen any time soon. So we removed it again. By the time all operations are implemented the test would basically enforce that we don't delete any of the operations and hook them up correctly. However, that's already enforced via the individual integration tests. So it's a very low value test. --- 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 #646: GEODE-3213: Refactor ProtoBuf handler flow
Github user pivotal-amurmann commented on a diff in the pull request: https://github.com/apache/geode/pull/646#discussion_r128793895 --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java --- @@ -50,47 +38,14 @@ * @param ex - exception which should be logged * @return An error response containing the first three parameters. */ - public static ClientProtocol.Response createAndLogErrorResponse(String errorMessage, + public static ClientProtocol.ErrorResponse createAndLogFailureResponse(String errorMessage, --- End diff -- I think those were to changes that were made independently. Syncing this up right 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 #657: GEODE-3286: Failing to cleanup connections from Con...
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/657#discussion_r129689404
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java ---
@@ -279,26 +280,29 @@ protected void acceptConnection(Socket sock) throws
IOException, ConnectionExcep
// in our caller.
// no need to log error here since caller will log warning
- if (conn != null && !finishedConnecting) {
+ if (connection != null && !finishedConnecting) {
// we must be throwing from checkCancelInProgress so close the
connection
-
closeCon(LocalizedStrings.ConnectionTable_CANCEL_AFTER_ACCEPT.toLocalizedString(),
conn);
-conn = null;
+
closeCon(LocalizedStrings.ConnectionTable_CANCEL_AFTER_ACCEPT.toLocalizedString(),
+connection);
+connection = null;
}
}
-if (conn != null) {
+if (connection != null) {
synchronized (this.receivers) {
-this.owner.stats.incReceivers();
+this.owner.getStats().incReceivers();
if (this.closed) {
closeCon(LocalizedStrings.ConnectionTable_CONNECTION_TABLE_NO_LONGER_IN_USE
- .toLocalizedString(), conn);
+ .toLocalizedString(), connection);
return;
}
-this.receivers.add(conn);
+if (!connection.isSocketClosed()) {
--- End diff --
Maybe once this gets squashed into a single commit the explanation can be
added to the commit message to be preserved with the code? It's always awesome
to do a `git annotate` and get a great explanation
---
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 pivotal-amurmann commented on the issue: https://github.com/apache/geode/pull/663 I am sure it would be hard, but could we please write a unit test for the change that was made to `DLockService#unlock`? --- 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 pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/673#discussion_r130642543
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
---
@@ -0,0 +1,98 @@
+/*
+ * 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.Properties;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+public class GetAvailableServersOperationHandler implements
+OperationHandler {
+
+ @Override
+ public Result process(
+ SerializationService serializationService,
ServerAPI.GetAvailableServersRequest request,
+ Cache cache) {
+
+InternalDistributedSystem distributedSystem =
+(InternalDistributedSystem) cache.getDistributedSystem();
+Properties properties = distributedSystem.getProperties();
+String locatorsString =
properties.getProperty(ConfigurationProperties.LOCATORS);
--- End diff --
Could we pull the entire calculation of locators out either into a private
method or a service object? I think that would make it much easier to
understand what this method is doing.
---
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 pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/673#discussion_r130644766
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
---
@@ -0,0 +1,98 @@
+/*
+ * 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.Properties;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+public class GetAvailableServersOperationHandler implements
+OperationHandler {
+
+ @Override
+ public Result process(
+ SerializationService serializationService,
ServerAPI.GetAvailableServersRequest request,
+ Cache cache) {
+
+InternalDistributedSystem distributedSystem =
+(InternalDistributedSystem) cache.getDistributedSystem();
+Properties properties = distributedSystem.getProperties();
+String locatorsString =
properties.getProperty(ConfigurationProperties.LOCATORS);
+
+HashSet locators = new HashSet();
+StringTokenizer stringTokenizer = new StringTokenizer(locatorsString,
",");
+while (stringTokenizer.hasMoreTokens()) {
+ String locator = stringTokenizer.nextToken();
+ if (StringUtils.isNotEmpty(locator)) {
+locators.add(new DistributionLocatorId(locator));
+ }
+}
+
+TcpClient tcpClient = getTcpClient();
+for (DistributionLocatorId locator : locators) {
--- End diff --
this is not super pretty. What do you think about using Stream#findFirst
for 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 #673: GEODE-3284: New flow: getAvailableServers
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/673#discussion_r130645759
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
---
@@ -0,0 +1,98 @@
+/*
+ * 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.Properties;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+public class GetAvailableServersOperationHandler implements
+OperationHandler {
+
+ @Override
+ public Result process(
+ SerializationService serializationService,
ServerAPI.GetAvailableServersRequest request,
+ Cache cache) {
+
+InternalDistributedSystem distributedSystem =
+(InternalDistributedSystem) cache.getDistributedSystem();
+Properties properties = distributedSystem.getProperties();
+String locatorsString =
properties.getProperty(ConfigurationProperties.LOCATORS);
+
+HashSet locators = new HashSet();
+StringTokenizer stringTokenizer = new StringTokenizer(locatorsString,
",");
+while (stringTokenizer.hasMoreTokens()) {
+ String locator = stringTokenizer.nextToken();
+ if (StringUtils.isNotEmpty(locator)) {
+locators.add(new DistributionLocatorId(locator));
+ }
+}
+
+TcpClient tcpClient = getTcpClient();
+for (DistributionLocatorId locator : locators) {
+ try {
+return getGetAvailableServersFromLocator(tcpClient,
locator.getHost());
+ } catch (IOException | ClassNotFoundException e) {
+// try the next locator
+ }
+}
+return Failure
+.of(BasicTypes.ErrorResponse.newBuilder().setMessage("Unable to
find a locator").build());
+ }
+
+ private Result
getGetAvailableServersFromLocator(
+ TcpClient tcpClient, InetSocketAddress address) throws IOException,
ClassNotFoundException {
+GetAllServersResponse getAllServersResponse = (GetAllServersResponse)
tcpClient
+.requestToServer(address, new GetAllServersRequest(), 1000, true);
+Collection servers =
+(Collection)
getAllServersResponse.getServers().stream()
+.map(serverLocation ->
getServerProtobufMessage((ServerLocation) serverLocation))
+.collect(Collectors.toList());
+ServerAPI.GetAvailableServersResponse.Builder builder =
+
ServerAPI.GetAvailableServersResponse.newBuilder().addAllServers(servers);
--- End diff --
could we incrementally build this up as part of the stream processing?
---
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 wish
[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers
Github user pivotal-amurmann commented on a diff in the pull request: https://github.com/apache/geode/pull/673#discussion_r130646177 --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java --- @@ -140,7 +141,7 @@ /** * This creates a MessageHeader - * + * --- End diff -- what's up with adding all this white trailing white space? Is this from spotless? --- 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 pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/673#discussion_r130646425
--- Diff: geode-protobuf/src/main/proto/clientProtocol.proto ---
@@ -64,15 +63,14 @@ message Response {
GetAllResponse getAllResponse = 5;
RemoveResponse removeResponse = 6;
RemoveAllResponse removeAllResponse = 7;
-ListKeysResponse listKeysResponse = 8;
ErrorResponse errorResponse = 13;
CreateRegionResponse createRegionResponse = 20;
DestroyRegionResponse destroyRegionResponse = 21;
-PingResponse pingResponse = 41;
-GetServersResponse getServersResponse = 42;
+//PingResponse pingResponse = 41;
--- End diff --
same as 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 #673: GEODE-3284: New flow: getAvailableServers
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/673#discussion_r130646340
--- Diff: geode-protobuf/src/main/proto/clientProtocol.proto ---
@@ -43,13 +43,12 @@ message Request {
GetAllRequest getAllRequest = 5;
RemoveRequest removeRequest = 6;
RemoveAllRequest removeAllRequest = 7;
-ListKeysRequest listKeysRequest = 8;
CreateRegionRequest createRegionRequest = 21;
DestroyRegionRequest destroyRegionRequest = 22;
-PingRequest pingRequest = 41;
-GetServersRequest getServersRequest = 42;
+//PingRequest pingRequest = 41;
--- End diff --
what are you saving it for?
---
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 pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/673#discussion_r130646564
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/GetAvailableServersDUnitTest.java
---
@@ -0,0 +1,108 @@
+/*
+ * 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;
+
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.protocol.exception.InvalidProtocolMessageException;
+import org.apache.geode.protocol.protobuf.ClientProtocol;
+import org.apache.geode.protocol.protobuf.ServerAPI;
+import
org.apache.geode.protocol.protobuf.serializer.ProtobufProtocolSerializer;
+import
org.apache.geode.protocol.protobuf.utilities.ProtobufRequestUtilities;
+import org.apache.geode.protocol.protobuf.utilities.ProtobufUtilities;
+import org.apache.geode.test.dunit.DistributedTestUtils;
+import org.apache.geode.test.dunit.Host;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.cache.internal.JUnit4CacheTestCase;
+import
org.apache.geode.test.dunit.rules.DistributedRestoreSystemProperties;
+import org.apache.geode.test.junit.categories.DistributedTest;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.IOException;
+import java.net.Socket;
+
+import static org.junit.Assert.assertEquals;
+
+@Category(DistributedTest.class)
+public class GetAvailableServersDUnitTest extends JUnit4CacheTestCase {
+
+ @Rule
+ public DistributedRestoreSystemProperties
distributedRestoreSystemProperties =
+ new DistributedRestoreSystemProperties();
+
+ @Before
+ public void setup() {
+
+ }
+
+ @Test
+ public void testGetAllAvailableServersRequest()
+ throws IOException, InvalidProtocolMessageException {
+Host host = Host.getHost(0);
+VM vm0 = host.getVM(0);
+VM vm1 = host.getVM(1);
+VM vm2 = host.getVM(2);
+
+int locatorPort = DistributedTestUtils.getDUnitLocatorPort();
+
+// int cacheServer1Port = vm0.invoke("Start Cache1", () ->
startCacheWithCacheServer());
--- End diff --
let's get rid off 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 #673: GEODE-3284: New flow: getAvailableServers
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/673#discussion_r130646935
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/GetAvailableServersDUnitTest.java
---
@@ -0,0 +1,108 @@
+/*
+ * 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;
+
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.protocol.exception.InvalidProtocolMessageException;
+import org.apache.geode.protocol.protobuf.ClientProtocol;
+import org.apache.geode.protocol.protobuf.ServerAPI;
+import
org.apache.geode.protocol.protobuf.serializer.ProtobufProtocolSerializer;
+import
org.apache.geode.protocol.protobuf.utilities.ProtobufRequestUtilities;
+import org.apache.geode.protocol.protobuf.utilities.ProtobufUtilities;
+import org.apache.geode.test.dunit.DistributedTestUtils;
+import org.apache.geode.test.dunit.Host;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.cache.internal.JUnit4CacheTestCase;
+import
org.apache.geode.test.dunit.rules.DistributedRestoreSystemProperties;
+import org.apache.geode.test.junit.categories.DistributedTest;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.IOException;
+import java.net.Socket;
+
+import static org.junit.Assert.assertEquals;
+
+@Category(DistributedTest.class)
+public class GetAvailableServersDUnitTest extends JUnit4CacheTestCase {
+
+ @Rule
+ public DistributedRestoreSystemProperties
distributedRestoreSystemProperties =
+ new DistributedRestoreSystemProperties();
+
+ @Before
+ public void setup() {
+
+ }
+
+ @Test
+ public void testGetAllAvailableServersRequest()
+ throws IOException, InvalidProtocolMessageException {
+Host host = Host.getHost(0);
+VM vm0 = host.getVM(0);
+VM vm1 = host.getVM(1);
+VM vm2 = host.getVM(2);
+
+int locatorPort = DistributedTestUtils.getDUnitLocatorPort();
+
+// int cacheServer1Port = vm0.invoke("Start Cache1", () ->
startCacheWithCacheServer());
+int cacheServer1Port = startCacheWithCacheServer();
+int cacheServer2Port = vm1.invoke("Start Cache2", () ->
startCacheWithCacheServer());
+int cacheServer3Port = vm2.invoke("Start Cache3", () ->
startCacheWithCacheServer());
+
+vm0.invoke(() -> {
+ Socket socket = new Socket(host.getHostName(), cacheServer1Port);
+ socket.getOutputStream().write(110);
+
+ ClientProtocol.Request.Builder protobufRequestBuilder =
+ ProtobufUtilities.createProtobufRequestBuilder();
+ ClientProtocol.Message getAvailableServersRequestMessage =
+
ProtobufUtilities.createProtobufMessage(ProtobufUtilities.createMessageHeader(1233445),
--- End diff --
nit pick: Could we move the first param to the line below, so that is lines
up with the second one? That would make this much more readable. Not sure if
spotless hates that though.
---
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 pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/673#discussion_r130647800
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandlerJUnitTest.java
---
@@ -0,0 +1,131 @@
+/*
+ * 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.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.cache.GemFireCacheImpl;
+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.ServerAPI.GetAvailableServersResponse;
+import org.apache.geode.protocol.protobuf.Success;
+import
org.apache.geode.protocol.protobuf.utilities.ProtobufRequestUtilities;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Properties;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+@Category(UnitTest.class)
+public class GetAvailableServersOperationHandlerJUnitTest extends
OperationHandlerJUnitTest {
+
+ private TcpClient mockTCPClient;
+
+ @Before
+ public void setUp() throws Exception {
+super.setUp();
+
+operationHandler = mock(GetAvailableServersOperationHandler.class);
+cacheStub = mock(GemFireCacheImpl.class);
+when(operationHandler.process(any(), any(),
any())).thenCallRealMethod();
+InternalDistributedSystem mockDistributedSystem =
mock(InternalDistributedSystem.class);
--- End diff --
Can we have some blank lines as separators between segments that pertain to
a single mock? Right now this is a fairly impenetrable wall of text.
---
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 pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/676#discussion_r130920986
--- 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 --
This originally started at "1" and was followed by "100". After I protested
it was changed to "001". After more discussion we ended up with the "1000" and
"2000" ranges to accommodate for sub-ranges.
---
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 pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/683#discussion_r131977004
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
---
@@ -1433,29 +1412,7 @@ public boolean lockInterruptibly(final Object name,
final long waitTimeMillis,
int lockId = -1;
incActiveLocks();
-int loopCount = 0;
while (keepTrying) {
--- End diff --
This `keepTrying` thing is very unclear because it hides what the actual
decision is based on. It seems like we keep trying until either the `waitLimit`
has been exceeded or the lock has been acquired. Could we split `keepTrying`
out into `boolean lockAcquired` and `boolean waitLimitExceeded`? Maybe the
check for `waitTimeLimit` doesn't even need a variable but could be checked by
the `while`?
---
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 pivotal-amurmann commented on a diff in the pull request: https://github.com/apache/geode/pull/683#discussion_r131974883 --- 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 -- how comes we are keeping this debug harness but not the others? --- 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 #698: Mark ProtoBuf interface as experimental
GitHub user pivotal-amurmann opened a pull request: https://github.com/apache/geode/pull/698 Mark ProtoBuf interface as experimental 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? 3042 - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? N/A - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? N/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/pivotal-amurmann/geode chore/GEODE-3402 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/698.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 #698 commit 26425bb0ce0cb0936fe0c0534abf5f1f5eac0e05 Author: Bruce Schuchardt Date: 2017-08-08T23:29:29Z Mark ProtoBuf interface as experimental Signed-off-by: Alexander Murmann --- 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 #700: GEODE-3386 - Make KeyedErrorResponse & ErrorRespons...
GitHub user pivotal-amurmann opened a pull request: https://github.com/apache/geode/pull/700 GEODE-3386 - Make KeyedErrorResponse & ErrorResponse siblings Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x] Is your initial contribution a single, squashed commit? - [ ] 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)? N/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/pivotal-amurmann/geode feature/GEODE-3386 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/700.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 #700 commit 81d7a34ef695738c61bf3d288b9ff01f054c30f5 Author: Alexander Murmann Date: 2017-08-09T18:52:17Z GEODE-3386 - Make KeyedErrorResponse & ErrorResponse siblings --- 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 #700: GEODE-3386 - Make KeyedErrorResponse & ErrorRespons...
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/700#discussion_r133000153
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
---
@@ -59,13 +59,14 @@
}
return
Success.of(RegionAPI.GetAllResponse.newBuilder().addAllEntries(entries).build());
} catch (UnsupportedEncodingTypeException ex) {
- return Failure.of(BasicTypes.ErrorResponse.newBuilder()
- .setErrorCode(ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue)
- .setMessage("Encoding not supported.").build());
+ int errorCode = ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue;
+ String message = "Encoding not supported.";
+ return
Failure.of(ProtobufResponseUtilities.makeErrorResponse(errorCode, message));
--- End diff --
good catch! The variables were supposed to be temporary to enable
IntelliJ's refactoring.
---
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 #700: GEODE-3386 - Make KeyedErrorResponse & ErrorRespons...
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/700#discussion_r133001811
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
---
@@ -79,9 +81,10 @@
private BasicTypes.KeyedErrorResponse
buildAndLogKeyedError(BasicTypes.Entry entry,
ProtocolErrorCode errorCode, String message, Exception ex) {
logger.error(message, ex);
-BasicTypes.ErrorResponse errorResponse =
BasicTypes.ErrorResponse.newBuilder()
-.setErrorCode(errorCode.codeValue).setMessage(message).build();
-return
BasicTypes.KeyedErrorResponse.newBuilder().setKey(entry.getKey()).setError(errorResponse)
+
+return
BasicTypes.KeyedErrorResponse.newBuilder().setKey(entry.getKey())
+.setError(
+
BasicTypes.Error.newBuilder().setErrorCode(errorCode.codeValue).setMessage(message))
--- End diff --
yeah it did ð
Tried to reformat it, but it insists on making this bad.
---
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 pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/707#discussion_r133246611
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
---
@@ -22,59 +22,89 @@
import java.io.IOException;
import java.net.Socket;
+import java.util.HashMap;
import java.util.Iterator;
+import java.util.Map;
import java.util.ServiceLoader;
-import javax.management.ServiceNotFoundException;
/**
* Creates instances of ServerConnection based on the connection mode
provided.
*/
public class ServerConnectionFactory {
- private static ClientProtocolMessageHandler protobufProtocolHandler;
- private static final Object protocolLoadLock = new Object();
+ private ClientProtocolMessageHandler protobufProtocolHandler;
+ private Map> authenticators
= null;
- private static ClientProtocolMessageHandler
findClientProtocolMessageHandler() {
+ public ServerConnectionFactory() {}
+
+ private synchronized void initializeAuthenticatorsMap() {
+if (authenticators != null) {
+ return;
+}
+authenticators = new HashMap<>();
+ServiceLoader loader =
ServiceLoader.load(StreamAuthenticator.class);
+for (StreamAuthenticator streamAuthenticator : loader) {
+ authenticators.put(streamAuthenticator.implementationID(),
streamAuthenticator.getClass());
+}
+ }
+
+ private synchronized ClientProtocolMessageHandler
initializeMessageHandler() {
if (protobufProtocolHandler != null) {
return protobufProtocolHandler;
}
+ServiceLoader loader =
+ServiceLoader.load(ClientProtocolMessageHandler.class);
+Iterator iterator = loader.iterator();
-synchronized (protocolLoadLock) {
- if (protobufProtocolHandler != null) {
-return protobufProtocolHandler;
- }
-
- ServiceLoader loader =
- ServiceLoader.load(ClientProtocolMessageHandler.class);
- Iterator iterator = loader.iterator();
-
- if (!iterator.hasNext()) {
-throw new ServiceLoadingFailureException(
-"ClientProtocolMessageHandler implementation not found in
JVM");
- }
+if (!iterator.hasNext()) {
+ throw new ServiceLoadingFailureException(
+ "There is no ClientProtocolMessageHandler implementation found
in JVM");
+}
- ClientProtocolMessageHandler returnValue = iterator.next();
+protobufProtocolHandler = iterator.next();
+return protobufProtocolHandler;
+ }
- if (iterator.hasNext()) {
+ private StreamAuthenticator findStreamAuthenticator(String
implementationID) {
+if (authenticators == null) {
+ initializeAuthenticatorsMap();
+}
+Class streamAuthenticatorClass =
+authenticators.get(implementationID);
+if (streamAuthenticatorClass == null) {
+ throw new ServiceLoadingFailureException(
+ "Could not find implementation for StreamAuthenticator with
implementation ID "
+ + implementationID);
+} else {
+ try {
+return streamAuthenticatorClass.newInstance();
+ } catch (InstantiationException | IllegalAccessException e) {
throw new ServiceLoadingFailureException(
-"Multiple service implementations found for
ClientProtocolMessageHandler");
--- End diff --
Where are we now handling the case of having multiple implementations?
---
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 #702: GEODE-3416: Reduce synchronization blockages in Soc...
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/702#discussion_r133252327
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/net/SocketCloser.java ---
@@ -96,46 +99,56 @@ public int getMaxThreads() {
return this.asyncClosePoolMaxThreads;
}
- private ThreadPoolExecutor getAsyncThreadExecutor(String address) {
-synchronized (asyncCloseExecutors) {
- ThreadPoolExecutor pool = asyncCloseExecutors.get(address);
- if (pool == null) {
-final ThreadGroup tg =
LoggingThreadGroup.createThreadGroup("Socket asyncClose", logger);
-ThreadFactory tf = new ThreadFactory() {
- public Thread newThread(final Runnable command) {
-Thread thread = new Thread(tg, command);
-thread.setDaemon(true);
-return thread;
- }
-};
-BlockingQueue workQueue = new
LinkedBlockingQueue();
-pool = new ThreadPoolExecutor(this.asyncClosePoolMaxThreads,
this.asyncClosePoolMaxThreads,
-this.asyncClosePoolKeepAliveSeconds, TimeUnit.SECONDS,
workQueue, tf);
-pool.allowCoreThreadTimeOut(true);
-asyncCloseExecutors.put(address, pool);
+ private ExecutorService getAsyncThreadExecutor(String address) {
+ExecutorService executorService = asyncCloseExecutors.get(address);
+if (executorService == null) {
+ // To be used for pre-1.8 jdk releases.
+ // createThreadPool();
+
+ executorService =
Executors.newWorkStealingPool(asyncClosePoolMaxThreads);
+
+ ExecutorService previousThreadPoolExecutor =
+ asyncCloseExecutors.putIfAbsent(address, executorService);
+
+ if (previousThreadPoolExecutor != null) {
+executorService.shutdownNow();
+return previousThreadPoolExecutor;
}
- return pool;
}
+return executorService;
+ }
+
+ /**
+ * @deprecated this method is to be used for pre 1.8 jdk.
--- End diff --
I always appreciate it as a consumer if deprecations contain information
about what the new solution is for the problem that the deprecated code was
solving.
---
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 #716: GEODE-3406: Locator accepts Protobuf requests
GitHub user pivotal-amurmann opened a pull request:
https://github.com/apache/geode/pull/716
GEODE-3406: Locator accepts Protobuf requests
Also addresses GEODE-3400, GEODE-3399
This allows the locator to respond to Protobuf requests. Currently it
will only be able to respond to getAvailableServers.
To enable this we are introducing a new value of "0" that will be sent
in place of the Gossip version. After it we expect the same magic byte
("110") as in AcceptorImpl.
This also is gated by the `geode.feature-protobuf-protocol` system
property.
The getAvailableServers request handler now uses the locator directly,
since we are on the locator.
Signed-off-by: Brian Rowe
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [âï¸ ] Is there a JIRA ticket associated with this PR? Is it
referenced in the commit message?
GEODE-3400, GEODE-3399, GEODE-3406
- [âï¸ ] Has your PR been rebased against the latest commit within the
target branch (typically `develop`)?
- [âï¸] Is your initial contribution a single, squashed commit?
- [âï¸] Does `gradlew build` run cleanly?
- [âï¸] Have you written or updated unit tests to verify your changes?
- [x] 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)?
N/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.
@kohlmu-pivotal @bschuchardt @galen-pivotal @WireBaron @hiteshk25
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pivotal-amurmann/geode
feature/GEODE-3400/locator_support_protobuf
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/716.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 #716
commit a1d10ffdc6eda365eafa11d055cbce5096abcec2
Author: Alexander Murmann
Date: 2017-08-14T22:08:14Z
GEODE-3406: Locator accepts Protobuf requests
Also addresses GEODE-3400, GEODE-3399
This allows the locator to respond to Protobuf requests. Currently it
will only be able to respond to getAvailableServers.
To enable this we are introducing a new value of "0" that will be sent
in place of the Gossip version. After it we expect the same magic byte
("110") as in AcceptorImpl.
This also is gated by the `geode.feature-protobuf-protocol` system
property.
The getAvailableServers request handler now uses the locator directly,
since we are on the locator.
Signed-off-by: Brian Rowe
---
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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133777949
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
---
@@ -334,42 +342,46 @@ protected void run() {
* fix for bug 33711 - client requests are spun off to another thread
for processing. Requests are
* synchronized in processGossip.
*/
- private void processRequest(final Socket sock) {
+ private void processRequest(final Socket socket) {
executor.execute(() -> {
long startTime = DistributionStats.getStatTime();
DataInputStream input = null;
Object request, response;
try {
-sock.setSoTimeout(READ_TIMEOUT);
-getSocketCreator().configureServerSSLSocket(sock);
+socket.setSoTimeout(READ_TIMEOUT);
+getSocketCreator().configureServerSSLSocket(socket);
try {
- input = new DataInputStream(sock.getInputStream());
+ input = new DataInputStream(socket.getInputStream());
} catch (StreamCorruptedException e) {
// Some garbage can be left on the socket stream
// if a peer disappears at exactly the wrong moment.
log.debug("Discarding illegal request from "
- + (sock.getInetAddress().getHostAddress() + ":" +
sock.getPort()), e);
+ + (socket.getInetAddress().getHostAddress() + ":" +
socket.getPort()), e);
return;
}
-int gossipVersion = readGossipVersion(sock, input);
+int gossipVersion = readGossipVersion(socket, input);
short versionOrdinal;
+if (gossipVersion == NON_GOSSIP_REQUEST_VERSION) {
+ if (input.readUnsignedByte() ==
AcceptorImpl.PROTOBUF_CLIENT_SERVER_PROTOCOL
+ && Boolean.getBoolean("geode.feature-protobuf-protocol")) {
+ClientProtocolMessageHandler messageHandler =
ClientProtocolMessageHandlerLoader.load();
+messageHandler.receiveMessage(input, socket.getOutputStream(),
+new ExecutionContext(internalLocator));
--- End diff --
after this we need to close the socket. --Hitesh
---
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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133844781
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
---
@@ -334,42 +342,46 @@ protected void run() {
* fix for bug 33711 - client requests are spun off to another thread
for processing. Requests are
* synchronized in processGossip.
*/
- private void processRequest(final Socket sock) {
+ private void processRequest(final Socket socket) {
executor.execute(() -> {
long startTime = DistributionStats.getStatTime();
DataInputStream input = null;
Object request, response;
try {
-sock.setSoTimeout(READ_TIMEOUT);
-getSocketCreator().configureServerSSLSocket(sock);
+socket.setSoTimeout(READ_TIMEOUT);
+getSocketCreator().configureServerSSLSocket(socket);
try {
- input = new DataInputStream(sock.getInputStream());
+ input = new DataInputStream(socket.getInputStream());
} catch (StreamCorruptedException e) {
// Some garbage can be left on the socket stream
// if a peer disappears at exactly the wrong moment.
log.debug("Discarding illegal request from "
- + (sock.getInetAddress().getHostAddress() + ":" +
sock.getPort()), e);
+ + (socket.getInetAddress().getHostAddress() + ":" +
socket.getPort()), e);
return;
}
-int gossipVersion = readGossipVersion(sock, input);
+int gossipVersion = readGossipVersion(socket, input);
short versionOrdinal;
+if (gossipVersion == NON_GOSSIP_REQUEST_VERSION) {
+ if (input.readUnsignedByte() ==
AcceptorImpl.PROTOBUF_CLIENT_SERVER_PROTOCOL
+ && Boolean.getBoolean("geode.feature-protobuf-protocol")) {
+ClientProtocolMessageHandler messageHandler =
ClientProtocolMessageHandlerLoader.load();
--- End diff --
I would really like to have this constant only defined once. Agreed that
AcceptorImpl is a bad spot for that. What would be a good spot?
---
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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133847176
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandlerJUnitTest.java
---
@@ -32,75 +30,48 @@
import org.junit.Test;
import org.junit.experimental.categories.Category;
-import java.io.IOException;
import java.util.ArrayList;
-import java.util.Properties;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyBoolean;
-import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@Category(UnitTest.class)
public class GetAvailableServersOperationHandlerJUnitTest extends
OperationHandlerJUnitTest {
--- End diff --
tried to remove it. It's quite painful. => probably the answer to your
question is "yes"
---
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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133847775
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientProtoclMessageHandlerLoader.java
---
@@ -0,0 +1,64 @@
+/*
+ * 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;
+
+import java.io.IOException;
+import java.net.Socket;
+import java.util.Iterator;
+import java.util.ServiceLoader;
+
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.tier.Acceptor;
+import org.apache.geode.internal.cache.tier.CachedRegionHelper;
+import org.apache.geode.internal.security.SecurityService;
+
+/**
+ * Creates instances of ServerConnection based on the connection mode
provided.
+ */
+public class ClientProtoclMessageHandlerLoader {
--- End diff --
definitely. just trying to be consistent with code that was already there.
---
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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r134002721
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
---
@@ -392,7 +388,7 @@ public void testLocatorUpdateIntervalZero() throws
Exception {
private void startFakeLocator() throws UnknownHostException,
IOException, InterruptedException {
server = new TcpServer(port, InetAddress.getLocalHost(), null, null,
handler, new FakeHelper(),
-Thread.currentThread().getThreadGroup(), "Tcp Server");
+Thread.currentThread().getThreadGroup(), "Tcp Server", null);
server.start();
Thread.sleep(500);
--- End diff --
It could, but we only really touched this file to account for the changed
method signature
---
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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r134005011
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
---
@@ -392,7 +388,7 @@ public void testLocatorUpdateIntervalZero() throws
Exception {
private void startFakeLocator() throws UnknownHostException,
IOException, InterruptedException {
server = new TcpServer(port, InetAddress.getLocalHost(), null, null,
handler, new FakeHelper(),
-Thread.currentThread().getThreadGroup(), "Tcp Server");
+Thread.currentThread().getThreadGroup(), "Tcp Server", null);
--- End diff --
We added another parameter "protobuf handler" for TcpServer constructor to
process protobuf messages. And it is nothing to do with existing code base and
is only relevant for Protobuf requests. That's why we are passing null for old
code.
---
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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user pivotal-amurmann commented on a diff in the pull request: https://github.com/apache/geode/pull/716#discussion_r134053063 --- Diff: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/InvalidExecutionContextException.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.internal.cache.tier.sockets; --- End diff -- It gets thrown by the execution context which gets passed in from TcpServer and GenericProtocolServerConnection --- 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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user pivotal-amurmann commented on a diff in the pull request: https://github.com/apache/geode/pull/716#discussion_r134066027 --- Diff: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/InvalidExecutionContextException.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.internal.cache.tier.sockets; --- End diff -- Where would be a better place for this? I am honestly just plain confused by the existing structure. --- 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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r135087207
--- Diff:
geode-core/src/test/java/org/apache/geode/test/dunit/standalone/DUnitLauncher.java
---
@@ -297,6 +297,8 @@ public Object call() throws IOException {
// able to do so successfully anyway
p.setProperty(DISABLE_AUTO_RECONNECT, "true");
+System.setProperty("geode.feature-protobuf-protocol", "true");
--- End diff --
Ideally we would not set this all the time. However, it would be a
non-trivial effort to only set this when we need it and the feature flag should
go away soon entirely.
---
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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r135089666
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
---
@@ -50,51 +37,23 @@
@Override
public Result process(
SerializationService serializationService,
ServerAPI.GetAvailableServersRequest request,
- Cache cache) {
-
-InternalDistributedSystem distributedSystem =
-(InternalDistributedSystem) cache.getDistributedSystem();
-Properties properties = distributedSystem.getProperties();
-String locatorsString =
properties.getProperty(ConfigurationProperties.LOCATORS);
+ MessageExecutionContext executionContext) throws
InvalidExecutionContextException {
-HashSet locators = new HashSet();
-StringTokenizer stringTokenizer = new StringTokenizer(locatorsString,
",");
-while (stringTokenizer.hasMoreTokens()) {
- String locator = stringTokenizer.nextToken();
- if (StringUtils.isNotEmpty(locator)) {
-locators.add(new DistributionLocatorId(locator));
- }
+InternalLocator locator = executionContext.getLocator();
+ArrayList serversFromSnapshot =
--- End diff --
I very much agree with Galen that this should be refactored. This is a big
demeter violation which is pointing at some worse code in ServerLocator which
currently know how to answered requests and how to get the information to
answer them. If that was split out into one class that can talk whatever
protocol it's talking and another class that can get information from the
locator this could get cleaned up quite a bit and also make unit tests much
easier. Since we are talking about switching to Netty that might be wasted
effort at this time. On the other hand we should extract the business logic
from the transport logic anyways when moving to Netty and doing this beforehand
might make that move easier.
---
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 #742: GEODE-3473: Initial commit of the internal package renamin...
Github user pivotal-amurmann commented on the issue: https://github.com/apache/geode/pull/742 This looks good. However, I'd like to understand what the plan is for moving the files to the location suggested by the package name/path. Maybe the Commit message could clarify that? --- 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 #719: GEODE-3447 Implement client authorization for the n...
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/719#discussion_r135153564
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java
---
@@ -41,20 +43,29 @@ public void receiveMessage(InputStream inputStream,
OutputStream outputStream,
properties.setProperty(ResourceConstants.USER_NAME,
authenticationRequest.getUsername());
properties.setProperty(ResourceConstants.PASSWORD,
authenticationRequest.getPassword());
+authorizer = null; // authenticating a new user clears current
authorizer
try {
Object principal = securityManager.authenticate(properties);
- authenticated = principal != null;
+ if (principal != null) {
+authorizer = new ProtobufSimpleAuthorizer(principal,
securityManager);
+ }
} catch (AuthenticationFailedException e) {
- authenticated = false;
+ authorizer = null;
}
-
AuthenticationAPI.SimpleAuthenticationResponse.newBuilder().setAuthenticated(authenticated)
+
AuthenticationAPI.SimpleAuthenticationResponse.newBuilder().setAuthenticated(isAuthenticated())
.build().writeDelimitedTo(outputStream);
}
@Override
public boolean isAuthenticated() {
-return authenticated;
+// note: an authorizer is only created if the user has been
authenticated
+return authorizer != null;
--- End diff --
this would be nicer as well if `Optional`
---
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 #719: GEODE-3447 Implement client authorization for the n...
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/719#discussion_r135153452
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
---
@@ -44,9 +46,15 @@ public ProtobufOpsProcessor(SerializationService
serializationService,
ClientProtocol.Response.Builder builder;
Result result;
try {
- result =
operationContext.getOperationHandler().process(serializationService,
- operationContext.getFromRequest().apply(request), context);
-} catch (InvalidExecutionContextException e) {
+ if
(context.getAuthorizer().authorize(operationContext.getAccessPermissionRequired()))
{
+result =
operationContext.getOperationHandler().process(serializationService,
+operationContext.getFromRequest().apply(request), context);
+ } else {
--- End diff --
We might want to log this. Logging all security related events can be a
life saver during or after a security incident.
---
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 #719: GEODE-3447 Implement client authorization for the n...
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/719#discussion_r135153161
--- Diff:
geode-core/src/main/java/org/apache/geode/security/StreamAuthorizer.java ---
@@ -0,0 +1,19 @@
+/*
+ * 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.security;
+
+public interface StreamAuthorizer {
--- End diff --
Does it make sense to remove the `Stream` part of this name? There seems to
be nothing about this interface that is related to streams at 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 pull request #719: GEODE-3447 Implement client authorization for the n...
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/719#discussion_r135154701
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/AuthorizationIntegrationTest.java
---
@@ -0,0 +1,206 @@
+/*
+ * 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;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.same;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.net.Socket;
+import java.util.Properties;
+import java.util.concurrent.TimeUnit;
+
+import org.awaitility.Awaitility;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.internal.AvailablePortHelper;
+import org.apache.geode.management.internal.security.ResourceConstants;
+import org.apache.geode.protocol.protobuf.AuthenticationAPI;
+import org.apache.geode.protocol.protobuf.ClientProtocol;
+import org.apache.geode.protocol.protobuf.ProtobufSerializationService;
+import org.apache.geode.protocol.protobuf.ProtocolErrorCode;
+import org.apache.geode.protocol.protobuf.RegionAPI;
+import
org.apache.geode.protocol.protobuf.serializer.ProtobufProtocolSerializer;
+import org.apache.geode.protocol.protobuf.utilities.ProtobufUtilities;
+import org.apache.geode.security.ResourcePermission;
+import org.apache.geode.security.SecurityManager;
+import
org.apache.geode.serialization.registry.exception.CodecAlreadyRegisteredForTypeException;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+
+@Category(IntegrationTest.class)
+public class AuthorizationIntegrationTest {
--- End diff --
Hugs for not adding this to the existing Roundtrip test! ð¤
---
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 #719: GEODE-3447 Implement client authorization for the n...
Github user pivotal-amurmann commented on a diff in the pull request: https://github.com/apache/geode/pull/719#discussion_r135153058 --- Diff: geode-core/src/main/java/org/apache/geode/security/StreamAuthenticator.java --- @@ -46,6 +44,14 @@ void receiveMessage(InputStream inputStream, OutputStream outputStream, boolean isAuthenticated(); /** + * Return an authorization object which can be used to determine which permissions this stream has + * according to the provided securityManager. + * + * Calling this before authentication has succeeded may result in a null return object. --- End diff -- Could we indicate this by using a Optional? That way we also force the consumer to deal with that case. --- 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 #737: GEODE-3503: Removal of Codec classes left behind.
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/737#discussion_r135156514
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
---
@@ -119,7 +119,14 @@ public void setup() throws Exception {
}
CacheFactory cacheFactory = new CacheFactory(properties);
-cacheFactory.set("mcast-port", "0"); // sometimes it isn't due to
other tests.
+cacheFactory.set(ConfigurationProperties.MCAST_PORT, "0"); //
sometimes it isn't due to other
+ // tests.
+cacheFactory.set(ConfigurationProperties.ENABLE_CLUSTER_CONFIGURATION,
"false"); // sometimes it
+
// isn't due to
+
// other tests.
--- End diff --
I realize this is following existing style, but could we be more explicit
in the comment and say what `it isn't`? Something like `sometimes it is not set
to "false" due to test pollution` would be clearer. Or Even just `guard against
test pollution`. II had to read the comment multiple times to understand what
was going 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 #719: GEODE-3447 Implement client authorization for the n...
Github user pivotal-amurmann commented on a diff in the pull request: https://github.com/apache/geode/pull/719#discussion_r135550580 --- Diff: geode-core/src/main/java/org/apache/geode/security/StreamAuthenticator.java --- @@ -46,6 +44,14 @@ void receiveMessage(InputStream inputStream, OutputStream outputStream, boolean isAuthenticated(); /** + * Return an authorization object which can be used to determine which permissions this stream has + * according to the provided securityManager. + * + * Calling this before authentication has succeeded may result in a null return object. --- End diff -- How am I gonna see the comment as a user of this? I think it's unreasonable to assume that every consumer of the class is gonna look at its code. --- 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 #747: GEODE-3514: Clean up locator and protobuf related c...
GitHub user pivotal-amurmann opened a pull request: https://github.com/apache/geode/pull/747 GEODE-3514: Clean up locator and protobuf related code * Check for feature flag only once * Simplify TcpServerFactory * Indicate RoundTripLocatorConnection test properly as DUnit test * Move InvalidExecutionContextException to exception package. Signed-off-by: Alexander Murmann Signed-off-by: Brian Rowe Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? It does not b/c develop is red. I will rebase again once that's fixed - [ ] 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/pivotal-amurmann/geode chore/GEODE-3514/locator_cleanup Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/747.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 #747 commit 68bc6dd7853245e49bfee874af41a80eb68af159 Author: Brian Rowe Date: 2017-08-24T19:09:05Z GEODE-3514: Clean up locator and protobuf related code * Check for feature flag only once * Simplify TcpServerFactory * Indicate RoundTripLocatorConnection test properly as DUnit test * Move InvalidExecutionContextException to exception package. Signed-off-by: Alexander Murmann Signed-off-by: Brian Rowe --- 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 #747: GEODE-3514: Clean up locator and protobuf related code
Github user pivotal-amurmann commented on the issue: https://github.com/apache/geode/pull/747 **DO NOT MERGE THIS YET. THIS IS STILL BASED ON A RED DEVELOP CHECKOUT** --- 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. ---
