[GitHub] geode pull request #483: GEODE-2853: Change of locator list request interval
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/483#discussion_r116248082
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
---
@@ -109,6 +110,7 @@ public void setUp() throws Exception {
@After
public void tearDown() {
+System.clearProperty(DistributionConfig.GEMFIRE_PREFIX +
"LOCATOR_UPDATE_INTERVAL");
--- End diff --
This can be replaced with
`@Rule
public final RestoreSystemProperties restoreSystemProperties = new
RestoreSystemProperties();`
---
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 #522: GEODE-2953: Imports optimized in every file with a wildcar...
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/522 I really don't think that we should be doing this. Adds no value other than import reorganization --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #404: Geode 2469
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/404 @yhilem @ggreen @bbaynes, there is currently a feature branch https://github.com/apache/geode/tree/feature/GEODE-2444, which started splitting the Redis Adapter into its own module. I would prefer this splitting of the Redis adapter into its own module to be part of the integration. --- 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 #474: GEODE-2788: Add official Socket timeout parameter when con...
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/474 @masaki-yamakawa I'm running precheckin. If it passes I'll merge this PR. --- 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 #325: merge new version
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/325 Is this PR still open or relevant? If not, could we please close 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 #621: GEODE-3129 - Added error messages to protobuf protocol
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/621 Yes, there was a PR related to this, but it was deleted and then this one created. In addition, there is/was a review in apache reviewboard since Jul,3 --- 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 kohlmu-pivotal opened a pull request: https://github.com/apache/geode/pull/630 GEODE-3141: GetRegion Operation implemented Added OperationHandlerJUnitTest.java as parents class of all OperationHandler tests. General clean up of all `public static final` fields 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)? ### 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. @galen-pivotal @hiteshk25 @bschuchardt @WireBaron @pivotal-amurmann You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-3141 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/630.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 #630 commit 0f0fa0c08b5a66200055a5fc1a008881f5be95ab Author: Udo Kohlmeyer Date: 2017-07-13T00:22:55Z GEODE-3141: GetRegion Operation implemented Added OperationHandlerJUnitTest.java as parents class of all OperationHandler tests. General clean up of all `public static final` fields --- 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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/630#discussion_r127289396
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java
---
@@ -33,7 +33,7 @@
@Override
public ClientProtocol.Response process(SerializationService
serializationService,
- ClientProtocol.Request request, Cache cache) {
+ ClientProtocol.Request request,
Cache cache) {
--- End diff --
I forgot to run spotlessApply. Travis confirms the the failure.
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/630#discussion_r127289486
--- 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 --
I'll amend and add the checks now, rather than later
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #630: GEODE-3141: GetRegion Operation implemented
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/630#discussion_r127289532
--- Diff: geode-protobuf/src/main/proto/basicTypes.proto ---
@@ -52,7 +52,12 @@ message CallbackArguments {
message Region {
string name = 1;
-// TODO: key, value types?
+string type = 2;
--- End diff --
I like that idea. Will amend
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/630#discussion_r127294096
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java
---
@@ -133,21 +134,38 @@
/**
* This will return the object encoded in a protobuf EncodedValue
- *
* @param serializationService - object which knows how to encode
objects for the protobuf
- *protocol {@link ProtobufSerializationService}
+ * protocol {@link ProtobufSerializationService}
* @param encodedValue - The value to be decoded
* @return the object encoded in the passed encodedValue
* @throws UnsupportedEncodingTypeException - There isn't a
SerializationType matching the
- * encodedValues type
+ * encodedValues type
* @throws CodecNotRegisteredForTypeException - There isn't a protobuf
codec for the
- * SerializationType matching the encodedValues type
+ * SerializationType matching the encodedValues type
*/
public static Object decodeValue(SerializationService
serializationService,
- BasicTypes.EncodedValue encodedValue)
+ BasicTypes.EncodedValue encodedValue)
throws UnsupportedEncodingTypeException,
CodecNotRegisteredForTypeException {
BasicTypes.EncodingType encoding = encodedValue.getEncodingType();
byte[] bytes = encodedValue.getValue().toByteArray();
return serializationService.decode(encoding, bytes);
}
+
+ public static BasicTypes.Region createRegionMessageFromRegion(Region
region) {
+RegionAttributes regionAttributes = region.getAttributes();
+BasicTypes.Region.Builder protoRegionBuilder =
BasicTypes.Region.newBuilder();
+
+protoRegionBuilder.setName(region.getName());
+protoRegionBuilder.setSize(region.size());
+
+
protoRegionBuilder.setPersisted(regionAttributes.getDataPolicy().withPersistence());
+
protoRegionBuilder.setKeyConstraint(regionAttributes.getKeyConstraint() == null
? ""
--- End diff --
Correct. Amended according for both Key and Value constraint
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/630#discussion_r127294137
--- 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 --
Resolved
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #630: GEODE-3141: GetRegion Operation implemented
Github user kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/630#discussion_r127294276 --- Diff: geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java --- @@ -14,6 +14,14 @@ */ package org.apache.geode.protocol.protobuf.operations; +import static org.mockito.Mockito.any; --- End diff -- I run the spotlessApply... I don't rely on Intellij formatting. --- 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 #633: GEODE-3170: Closed socket doesn't result in an infi...
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/633#discussion_r127540044
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/GenericProtocolServerConnection.java
---
@@ -68,9 +56,8 @@ protected void doOneMessage() {
messageHandler.receiveMessage(inputStream, outputStream,
this.getCache());
} catch (IOException e) {
logger.warn(e);
- // TODO?
+ this.setFlagProcessMessagesAsFalse(); // TODO: better shutdown.
--- End diff --
Can we raise a ticket for this to complete this TODO
---
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 #633: GEODE-3170: Closed socket doesn't result in an infinite lo...
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/633 @hiteshk25 @pivotal-amurmann --- 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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/630#discussion_r127761196
--- Diff: geode-protobuf/src/main/proto/region_API.proto ---
@@ -102,4 +102,14 @@ message GetRegionNamesRequest {
message GetRegionNamesResponse {
repeated string regions = 1;
-}
\ No newline at end of file
+}
+
+/* does a region exist? */
+message GetRegionRequest {
+string regionName = 1;
+}
+
+/* success will be true if the region exists */
--- End diff --
Updating the documentation of 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 #630: GEODE-3141: GetRegion Operation implemented
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/630#discussion_r128038401
--- Diff: geode-protobuf/src/main/proto/basicTypes.proto ---
@@ -52,7 +52,12 @@ message CallbackArguments {
message Region {
--- End diff --
One would want to avoid just dumping fields into the response object
without context. Given all fields are related to the Region, it is better
suited to use a Region message.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/646#discussion_r128388403
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
---
@@ -35,10 +37,9 @@
* @param errorMessage - description of the error
* @return An error response containing the above parameters
*/
- public static ClientProtocol.Response createErrorResponse(String
errorMessage) {
-ClientProtocol.ErrorResponse error =
-
ClientProtocol.ErrorResponse.newBuilder().setMessage(errorMessage).build();
-return
ClientProtocol.Response.newBuilder().setErrorResponse(error).build();
+ public static Failure createFailureResult(String errorMessage) {
--- End diff --
I disagree with this method description or its implementation. Either the
method name should state `createFailureResultWithErrorResponseForErrorMessage`
OR the signature becomes `createFailureResult(ErrorResponse errorResponse)`.
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/646#discussion_r128387458
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/operations/OperationContext.java
---
@@ -0,0 +1,57 @@
+/*
+ * 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;
+
+import org.apache.geode.protocol.protobuf.ClientProtocol;
+
+import java.util.function.Function;
+
+public class OperationContext {
--- End diff --
Would it make sense to maybe package restrict this class
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/646#discussion_r128386591
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java
---
@@ -64,11 +72,10 @@ public void setUp() throws Exception {
public void test_puttingTheEncodedEntryIntoRegion() throws
UnsupportedEncodingTypeException,
CodecNotRegisteredForTypeException,
CodecAlreadyRegisteredForTypeException {
PutRequestOperationHandler operationHandler = new
PutRequestOperationHandler();
-ClientProtocol.Response response =
+Result result =
operationHandler.process(serializationServiceStub,
generateTestRequest(), cacheStub);
-
Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.PUTRESPONSE,
-response.getResponseAPICase());
+Assert.assertNotNull(result);
--- End diff --
We should assert that we got back a Success Object rather than just result
!= null
---
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 kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/646#discussion_r128386349 --- Diff: geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java --- @@ -124,20 +128,19 @@ public void test_RegionThrowsClasscastException() throws CodecAlreadyRegisteredF when(regionMock.put(any(), any())).thenThrow(ClassCastException.class); PutRequestOperationHandler operationHandler = new PutRequestOperationHandler(); -ClientProtocol.Response response = +Result result = operationHandler.process(serializationServiceStub, generateTestRequest(), cacheStub); - Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.ERRORRESPONSE, -response.getResponseAPICase()); +Assert.assertTrue(result instanceof Failure); --- End diff -- Do we need to assert at the error message --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/646#discussion_r128386432
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java
---
@@ -99,23 +105,21 @@ public void test_codecNotRegistered() throws
CodecAlreadyRegisteredForTypeExcept
TEST_KEY.getBytes(Charset.forName("UTF-8".thenThrow(exception);
PutRequestOperationHandler operationHandler = new
PutRequestOperationHandler();
-ClientProtocol.Response response =
+Result result =
operationHandler.process(serializationServiceStub,
generateTestRequest(), cacheStub);
-
Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.ERRORRESPONSE,
-response.getResponseAPICase());
+Assert.assertTrue(result instanceof Failure);
}
@Test
public void test_RegionNotFound() throws
CodecAlreadyRegisteredForTypeException,
UnsupportedEncodingTypeException, CodecNotRegisteredForTypeException
{
when(cacheStub.getRegion(TEST_REGION)).thenReturn(null);
PutRequestOperationHandler operationHandler = new
PutRequestOperationHandler();
-ClientProtocol.Response response =
+Result result =
operationHandler.process(serializationServiceStub,
generateTestRequest(), cacheStub);
-
Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.ERRORRESPONSE,
-response.getResponseAPICase());
+Assert.assertTrue(result instanceof Failure);
--- End diff --
We need to assert at least the error message. Maybe later on the error 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 #646: GEODE-3213: Refactor ProtoBuf handler flow
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/646#discussion_r128567505
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
---
@@ -67,30 +68,27 @@
*{@link ProtobufUtilities}
* @return A response indicating the passed value was found for a
incoming GetRequest
*/
- public static ClientProtocol.Response
createGetResponse(BasicTypes.EncodedValue resultValue) {
-RegionAPI.GetResponse getResponse =
-RegionAPI.GetResponse.newBuilder().setResult(resultValue).build();
-return
ClientProtocol.Response.newBuilder().setGetResponse(getResponse).build();
+ public static Success createGetResult(
+ BasicTypes.EncodedValue resultValue) {
+return new
Success<>(RegionAPI.GetResponse.newBuilder().setResult(resultValue).build());
--- End diff --
could we replace these types of calls with inline `Success.of(EncodedValue)`
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/646#discussion_r128814031
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java
---
@@ -0,0 +1,57 @@
+/*
+ * 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;
+
+import org.apache.geode.protocol.operations.OperationHandler;
+
+import java.util.function.Function;
+
+public class OperationContext {
--- End diff --
+1
---
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 #649: GEODE-2997: Change new protocol GetAllResponse.
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/649#discussion_r128847166
--- Diff: geode-protobuf/src/main/proto/clientProtocol.proto ---
@@ -66,7 +66,7 @@ message Response {
RemoveAllResponse removeAllResponse = 7;
ListKeysResponse listKeysResponse = 8;
-ErrorResponse errorResponse = 13;
+Error error = 13;
--- End diff --
Why has this been renamed to Error from ErrorResponse? Imo, we are
following the pattern (good or bad) that a ClientProtocol.Response contains an
Operation specific response. To now have a non-"Response" message sort of
breaks the mold.
---
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 #649: GEODE-2997: Change new protocol GetAllResponse.
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/649#discussion_r128848930
--- Diff: geode-protobuf/src/main/proto/basicTypes.proto ---
@@ -62,4 +62,14 @@ message Region {
message Server {
string url = 1;
-}
\ No newline at end of file
+}
+
+message Error {
+int32 errorCode = 1;
+string message = 2;
+}
+
+message ErrorEntry {
--- End diff --
Tbh, I'm not ecstatic about the name. I'd prefer `FailedEntry`
---
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 #535: GEODE-2986: Remove redundant log message
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/535 @AkihiroKitada we'll prioritize this merger. --- 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 #649: GEODE-2997: Change new protocol GetAllResponse.
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/649#discussion_r129628663
--- Diff: geode-protobuf/src/main/proto/clientProtocol.proto ---
@@ -66,7 +66,7 @@ message Response {
RemoveAllResponse removeAllResponse = 7;
ListKeysResponse listKeysResponse = 8;
-ErrorResponse errorResponse = 13;
+Error error = 13;
--- End diff --
To keep the "Response -> OperationalResponse" hierarchy, I'd prefer to keep
"ErrorResponse" that contains an "Error" object.
We _could_ inline it, but it detracts from the modus-operandi
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/657#discussion_r129879726
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java ---
@@ -1322,6 +1328,14 @@ private void createBatchSendBuffer() {
this.batchFlusher.start();
}
+ public void onIdleCancel() {
--- End diff --
Has this work been done yet?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #676: GEODE-3321: Adding ErrorCode values to protobuf pro...
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/676#discussion_r130928974
--- 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 --
I believe any error code structure decides how many characters are to be
used for an error code.
The fact that we represent them as integers is merely a "simplification"
choice over using Alpha numerics.
I don't believe starting the error code at 1000 is a problem
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/676#discussion_r130928395
--- Diff:
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandlerJUnitTest.java
---
@@ -95,8 +97,8 @@ public void
processReturnsUnsucessfulResponseForInvalidRegion()
operationHandler.process(serializationServiceStub, removeRequest,
cacheStub);
assertTrue(result instanceof Failure);
-org.junit.Assert.assertThat(result.getErrorMessage().getMessage(),
-CoreMatchers.containsString("Region"));
+assertEquals(ProtocolErrorCode.REGION_NOT_FOUND.codeValue,
--- End diff --
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #682: GEODE-3393: One-way SSL authentication fails with F...
GitHub user kohlmu-pivotal opened a pull request: https://github.com/apache/geode/pull/682 GEODE-3393: One-way SSL authentication fails with FileNotFoundException for $USER_HOME/.keystore Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [] Has 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? - [] 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/apache/geode feature/GEODE-3393 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/682.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 #682 commit 4f5262fa91e715efb5400507a19fd683a7078bf4 Author: Udo Kohlmeyer Date: 2017-08-03T21:13:06Z GEODE-3393: One-way SSL commit failing with userHome/.keystore not found commit 9a8700af71d14b22caefc026aab6bd01c99590ab Author: Udo Kohlmeyer Date: 2017-08-03T22:09:43Z GEODE-3393: One-way SSL commit failing with userHome/.keystore not found --- 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 #682: GEODE-3393: One-way SSL authentication fails with FileNotF...
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/682 @hiteshk25 @bschuchardt @pivotal-amurmann @WireBaron --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #702: GEODE-3416: Reduce synchronization blockages in Soc...
GitHub user kohlmu-pivotal opened a pull request: https://github.com/apache/geode/pull/702 GEODE-3416: Reduce synchronization blockages in SocketCloser. Remove synchronization blocks around HashMap. Replace that implementation with simpler ThreadPool that is not unbounded and does not grow as the number of remoteAddress (clients/peers) are added Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has 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? - [ ] 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/apache/geode feature/GEODE-3416 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/702.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 #702 commit 56d7707e12ecdecbc1881a9ac1575fac854e8fe9 Author: Udo Kohlmeyer Date: 2017-08-09T21:17:59Z GEODE-3416: Reduce synchronization blockages in SocketCloser. Remove synchronization blocks around HashMap. Replace that implementation with simpler ThreadPool that is not unbounded and does not grow as the number of remoteAddress (clients/peers) are added --- 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 #702: GEODE-3416: Reduce synchronization blockages in SocketClos...
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/702 @bschuchardt @galen-pivotal @hiteshk25 @pivotal-amurmann @dschneider-pivotal @WireBaron @kirklund --- 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 #702: GEODE-3416: Reduce synchronization blockages in SocketClos...
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/702 @bschuchardt @dschneider-pivotal is on the review of this PR. --- 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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/707#discussion_r132738771
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
---
@@ -63,6 +65,31 @@ private static ClientProtocolMessageHandler
findClientProtocolMessageHandler() {
}
}
+ private static Class
findStreamAuthenticator(
+ String implementationID) {
+if (authenticatorClass != null) {
+ return authenticatorClass;
+}
+
+synchronized (streamAuthenticatorLoadLock) {
--- End diff --
why do we prefer this approach over a synchronized method?
---
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 kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/707#discussion_r132738100 --- Diff: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NoOpStreamAuthenticator.java --- @@ -0,0 +1,43 @@ +/* + * 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 -- I'm not sure this class should live in this package. Is it not more related to security? --- 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 kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/707#discussion_r132737182 --- Diff: geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java --- @@ -1378,6 +1379,18 @@ */ String NAME = "name"; /** + * The authentication mode for the protobuf client-server protocol. + * + * + * Description: Specifies the authentication mode used by the geode-protobuf module. + * + * Default: "NOOP" + * + * Allowed values: "NOOP" "SIMPLE" + */ + @Experimental + String PROTOBUF_PROTOCOL_AUTHENTICATION_MODE = "protobuf-protocol-authentication-mode"; --- End diff -- This property is misleading. It is NOT a protobuf specific authentication mode. It is merely an authentication mechanism that uses protobuf underneath the covers. 1) A different property name is to be used 2) With this property, the feature toggle should also maybe be removed??!!? One cannot live without the other --- 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 kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/707#discussion_r132737475 --- Diff: geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java --- @@ -2433,6 +2433,25 @@ String getRedundancyZone(); /** + * @since Geode 1.??? TODO FIXME + */ + @ConfigAttribute(type = String.class) + String PROTOBUF_PROTOCOL_AUTHENTICATION_MODE_NAME = PROTOBUF_PROTOCOL_AUTHENTICATION_MODE; + String DEFAULT_PROTOBUF_PROTOCOL_AUTHENTICATION_MODE = "NOOP"; + + /** + * @since Geode 1.??? TODO FIXME + */ + @ConfigAttributeSetter(name = PROTOBUF_PROTOCOL_AUTHENTICATION_MODE) + void setProtobufProtocolAuthenticationMode(String authenticationMode); + + /** + * @since Geode 1.??? TODO FIXME + */ + @ConfigAttributeGetter(name = PROTOBUF_PROTOCOL_AUTHENTICATION_MODE) + String getProtobufProtocolAuthenticationMode(); + + /** --- End diff -- Without an official release version I don't think we should have any of the properties/configurations public until release. --- 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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/707#discussion_r132735527
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
---
@@ -63,6 +65,31 @@ private static ClientProtocolMessageHandler
findClientProtocolMessageHandler() {
}
}
+ private static Class
findStreamAuthenticator(
+ String implementationID) {
+if (authenticatorClass != null) {
+ return authenticatorClass;
+}
+
+synchronized (streamAuthenticatorLoadLock) {
+ if (authenticatorClass != null) {
+return authenticatorClass;
+ }
+
+ ServiceLoader loader =
ServiceLoader.load(StreamAuthenticator.class);
+
+ for (StreamAuthenticator classInstance : loader) {
+if (implementationID.equals(classInstance.implementationID())) {
+ return classInstance.getClass();
--- End diff --
Why do we get an instance of the StreamAuthenticator, just to call
`getClass` on it, and then later we create a new instance of class from this
instance.
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/707#discussion_r132735728
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
---
@@ -72,9 +99,15 @@ public static ServerConnection
makeServerConnection(Socket s, InternalCache c,
throw new IOException("Acceptor received unknown communication
mode: " + communicationMode);
} else {
protobufProtocolHandler = findClientProtocolMessageHandler();
-return new GenericProtocolServerConnection(s, c, helper, stats,
hsTimeout, socketBufferSize,
-communicationModeStr, communicationMode, acceptor,
protobufProtocolHandler,
-securityService);
+authenticatorClass = findStreamAuthenticator(
+
c.getInternalDistributedSystem().getConfig().getProtobufProtocolAuthenticationMode());
+try {
+ return new GenericProtocolServerConnection(s, c, helper, stats,
hsTimeout,
+ socketBufferSize, communicationModeStr, communicationMode,
acceptor,
+ protobufProtocolHandler, securityService,
authenticatorClass.newInstance());
--- End diff --
We should use the authenticator instance returned from the
`findClientProtocolMessageHandler`.
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/702#discussion_r133113689
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/net/SocketCloser.java ---
@@ -96,46 +99,55 @@ 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.
+ */
+ @Deprecated
+ private void createThreadPool() {
+ExecutorService executorService;
+final ThreadGroup threadGroup =
+LoggingThreadGroup.createThreadGroup("Socket asyncClose", logger);
+ThreadFactory threadFactory = new ThreadFactory() {
+ public Thread newThread(final Runnable command) {
+Thread thread = new Thread(threadGroup, command);
+thread.setDaemon(true);
+return thread;
+ }
+};
+
+executorService = new ThreadPoolExecutor(asyncClosePoolMaxThreads,
asyncClosePoolMaxThreads,
+asyncCloseWaitTime, asyncCloseWaitUnits, new
LinkedBlockingQueue<>(), threadFactory);
}
/**
* Call this method if you know all the resources in the closer for the
given address are no
* longer needed. Currently a thread pool is kept for each address and
if you know that an address
* no longer needs its pool then you should call this method.
*/
- public void releaseResourcesForAddress(String address) {
-synchronized (asyncCloseExecutors) {
- ThreadPoolExecutor pool = asyncCloseExecutors.get(address);
- if (pool != null) {
-pool.shutdown();
-asyncCloseExecutors.remove(address);
- }
-}
- }
- private boolean isClosed() {
-synchronized (asyncCloseExecutors) {
- return this.closed;
+ public void releaseResourcesForAddress(String address) {
+ExecutorService executorService = asyncCloseExecutors.remove(address);
+if (executorService != null) {
+ executorService.shutdown();
--- End diff --
given that remove is on the concurrent hashmap, one should only ever get
into this once. This should not suffer reentrancy problems.
---
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 #649: GEODE-2997: Change new protocol GetAllResponse.
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/649 @galen-pivotal is this PR active anymore? --- 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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/702#discussion_r133238417
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/net/SocketCloser.java ---
@@ -144,35 +156,22 @@ private boolean isClosed() {
* called then the asyncClose will be done synchronously.
*/
public void close() {
-synchronized (asyncCloseExecutors) {
+synchronized (closed) {
if (!this.closed) {
this.closed = true;
-for (ThreadPoolExecutor pool : asyncCloseExecutors.values()) {
- pool.shutdown();
-}
-asyncCloseExecutors.clear();
+ } else {
+return;
}
}
+for (ExecutorService executorService : asyncCloseExecutors.values()) {
+ executorService.shutdown();
+ asyncCloseExecutors.clear();
--- End diff --
Good catch.
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/702#discussion_r133238599
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/net/SocketCloser.java ---
@@ -96,46 +99,55 @@ 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.
+ */
+ @Deprecated
+ private void createThreadPool() {
+ExecutorService executorService;
+final ThreadGroup threadGroup =
+LoggingThreadGroup.createThreadGroup("Socket asyncClose", logger);
+ThreadFactory threadFactory = new ThreadFactory() {
+ public Thread newThread(final Runnable command) {
+Thread thread = new Thread(threadGroup, command);
+thread.setDaemon(true);
+return thread;
+ }
+};
+
+executorService = new ThreadPoolExecutor(asyncClosePoolMaxThreads,
asyncClosePoolMaxThreads,
+asyncCloseWaitTime, asyncCloseWaitUnits, new
LinkedBlockingQueue<>(), threadFactory);
}
/**
* Call this method if you know all the resources in the closer for the
given address are no
* longer needed. Currently a thread pool is kept for each address and
if you know that an address
* no longer needs its pool then you should call this method.
*/
- public void releaseResourcesForAddress(String address) {
-synchronized (asyncCloseExecutors) {
- ThreadPoolExecutor pool = asyncCloseExecutors.get(address);
- if (pool != null) {
-pool.shutdown();
-asyncCloseExecutors.remove(address);
- }
-}
- }
- private boolean isClosed() {
-synchronized (asyncCloseExecutors) {
- return this.closed;
+ public void releaseResourcesForAddress(String address) {
+ExecutorService executorService = asyncCloseExecutors.remove(address);
+if (executorService != null) {
+ executorService.shutdown();
--- End diff --
ExecutorService.shutdown does protect itself internally with locks, etc..
so we don't have to worry about multiple threads calling shutdown on the same
executor service
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/702#discussion_r133255222
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/net/SocketCloser.java ---
@@ -96,46 +99,55 @@ 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.
+ */
+ @Deprecated
+ private void createThreadPool() {
+ExecutorService executorService;
+final ThreadGroup threadGroup =
+LoggingThreadGroup.createThreadGroup("Socket asyncClose", logger);
+ThreadFactory threadFactory = new ThreadFactory() {
+ public Thread newThread(final Runnable command) {
+Thread thread = new Thread(threadGroup, command);
+thread.setDaemon(true);
+return thread;
+ }
+};
+
+executorService = new ThreadPoolExecutor(asyncClosePoolMaxThreads,
asyncClosePoolMaxThreads,
+asyncCloseWaitTime, asyncCloseWaitUnits, new
LinkedBlockingQueue<>(), threadFactory);
}
/**
* Call this method if you know all the resources in the closer for the
given address are no
* longer needed. Currently a thread pool is kept for each address and
if you know that an address
* no longer needs its pool then you should call this method.
*/
- public void releaseResourcesForAddress(String address) {
-synchronized (asyncCloseExecutors) {
- ThreadPoolExecutor pool = asyncCloseExecutors.get(address);
- if (pool != null) {
-pool.shutdown();
-asyncCloseExecutors.remove(address);
- }
-}
- }
- private boolean isClosed() {
-synchronized (asyncCloseExecutors) {
- return this.closed;
+ public void releaseResourcesForAddress(String address) {
+ExecutorService executorService = asyncCloseExecutors.remove(address);
+if (executorService != null) {
+ executorService.shutdown();
--- End diff --
Correct and agreed. When the cache closes, I imagine SocketCloser.close()
is called. Which means the closing of any socket post that would be done
in-line.
I don't know what the expected behavior is supposed to be, for the closing
of the sockets when that cache is closing. But you are correct that it could
mean that the closing of the socket could be holding up the shutting down of
the cache.
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/702#discussion_r133353995
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java ---
@@ -929,10 +929,6 @@ protected void removeEndpoint(DistributedMember
memberID, String reason,
owner.getDM().getMembershipManager().getShutdownCause());
}
}
-
- if (remoteAddress != null) {
-
this.socketCloser.releaseResourcesForAddress(remoteAddress.toString());
- }
--- End diff --
it does a missed revert...
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133775690
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java ---
@@ -76,7 +76,9 @@
*/
public interface InternalCache extends Cache, Extensible, CacheTime
{
- InternalDistributedMember getMyId();
+ default InternalDistributedMember getMyId() {
--- End diff --
so the default implementation will result in a potential NPE?!?!? I think
this should NOT have a default and each InternalCache should implement 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 #716: GEODE-3406: Locator accepts Protobuf requests
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133775860
--- 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 {
+ private static ClientProtocolMessageHandler protobufProtocolHandler;
--- End diff --
say no to statics
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #716: GEODE-3406: Locator accepts Protobuf requests
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133775201
--- 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));
+ } else {
+rejectUnknownProtocolConnection(socket, gossipVersion);
+return;
+ }
+}
if (gossipVersion <= getCurrentGossipVersion()
--- End diff --
why would this not be an 'else-if' construct. Or can the 'gossipVersion' be
valid for both paths?
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133775965
--- 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 {
+ private static ClientProtocolMessageHandler protobufProtocolHandler;
+ private static final Object protocolLoadLock = new Object();
--- End diff --
I think one can have a better implementation without statics and locking
objects.
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133774197
--- 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 --
Agreed. Is there a specific reason that TcpServer now needs to know about
AcceptorImpl?
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133781367
--- 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 {
- private TcpClient mockTCPClient;
+ public static final String HOSTNAME_1 = "hostname1";
--- End diff --
most likely these don't have to be 'public' AND 'static'. I believe
'private' and 'final' would be enough
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133778013
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ExecutionContext.java
---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.internal.InternalLocator;
+
+public class ExecutionContext {
--- End diff --
Would this not be a 'MessageExecutionContext' rather than a generic
'ExecutionContext'?
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133778501
--- 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 --
Why is it valid to pass in a 'null' InternalLocator?
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133779835
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
---
@@ -50,51 +37,19 @@
@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));
- }
-}
+ ExecutionContext executionContext) throws
InvalidExecutionContextException {
-TcpClient tcpClient = getTcpClient();
-for (DistributionLocatorId locator : locators) {
- try {
-return getGetAvailableServersFromLocator(tcpClient,
locator.getHost());
- } catch (IOException | ClassNotFoundException e) {
-// try the next locator
- }
-}
-return Failure.of(ProtobufResponseUtilities.makeErrorResponse(
-ProtocolErrorCode.DATA_UNREACHABLE.codeValue, "Unable to find a
locator"));
- }
+InternalLocator locator = executionContext.getLocator();
+ArrayList servers2 =
locator.getServerLocatorAdvisee().getLoadSnapshot().getServers(null);
--- End diff --
I think if a 'null' is passed into the 'getServers' method, it should at
least be explained that currently the server group functionality is not
supported. Then a TODO could be added or a ticket can be raised to make sure
this is not missed when server groups are supported
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133778667
--- 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 --
Could this possibly be replaced with an Awaitility statement rather than a
generic Thread.sleep which is more error prone
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133780406
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
---
@@ -50,51 +37,19 @@
@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));
- }
-}
+ ExecutionContext executionContext) throws
InvalidExecutionContextException {
-TcpClient tcpClient = getTcpClient();
-for (DistributionLocatorId locator : locators) {
- try {
-return getGetAvailableServersFromLocator(tcpClient,
locator.getHost());
- } catch (IOException | ClassNotFoundException e) {
-// try the next locator
- }
-}
-return Failure.of(ProtobufResponseUtilities.makeErrorResponse(
-ProtocolErrorCode.DATA_UNREACHABLE.codeValue, "Unable to find a
locator"));
- }
+InternalLocator locator = executionContext.getLocator();
+ArrayList servers2 =
locator.getServerLocatorAdvisee().getLoadSnapshot().getServers(null);
--- End diff --
why 'servers2'? What does the numeric denote? Maybe a different descriptive
variable name is required... like 'availableServers'
---
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 kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/716#discussion_r133783618 --- 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; --- End diff -- Do we maybe need to remove some imports? --- 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 kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/716#discussion_r133782267 --- 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. --- End diff -- I'm not sure that this comment is valid anymore... Something has to be said for not copy-pasting. --- 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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r133783516
--- 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 --
Do we not prefer 'Factories' over 'Loaders'?
---
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 #717: Feature/geode 3411 Monitor the neighbour JVM using neihbou...
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/717 @aravindmusigumpula could you please explain why this feature is required and why the member now has to adhere to his neighbor's member-timeout. What problem is this change addressing and how would this change improve the member-timeout suspect member notification. --- 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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r134005525
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
---
@@ -143,11 +150,12 @@
public TcpServer(int port, InetAddress bind_address, Properties
sslConfig,
DistributionConfigImpl cfg, TcpHandler handler, PoolStatHelper
poolHelper,
- ThreadGroup threadGroup, String threadName) {
+ ThreadGroup threadGroup, String threadName, InternalLocator
internalLocator) {
--- End diff --
Is there a reason why we pass in the specific `InternalLocator` rather than
a more generic `Locator`
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r134007044
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ExecutionContext.java
---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.internal.InternalLocator;
+
+public class ExecutionContext {
+ private Cache cache;
+ private InternalLocator locator;
--- End diff --
I think we should deal with the `Locator` interface rather than the
concrete implementation
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r134006932
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ExecutionContext.java
---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.internal.InternalLocator;
+
+public class ExecutionContext {
+ private Cache cache;
+ private InternalLocator locator;
+
+ public ExecutionContext(Cache cache) {
+this.cache = cache;
+ }
+
+ public ExecutionContext(InternalLocator locator) {
+this.locator = locator;
+ }
+
+ // This throws if the cache isn't present because we know that non of
the callers can take any
+ // reasonable action if the cache is not present
+ public Cache getCache() throws InvalidExecutionContextException {
+if (cache != null) {
+ return cache;
+} else {
+ throw new InvalidExecutionContextException(
+ "Execution context's cache was accessed but isn't present. Did
this happen on a locator? Operations on the locator should not try to operate
on a cache");
+}
+ }
+
+ // This throws if the locator isn't present because we know that non of
the callers can take any
+ // reasonable action if the locator is not present
+ public InternalLocator getLocator() throws
InvalidExecutionContextException {
--- End diff --
I think we should return a `Locator` rather than an `InternalLocator`
---
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 kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/716#discussion_r134007487 --- Diff: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ExecutionContext.java --- @@ -0,0 +1,54 @@ +/* + * 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 -- Is this class maybe in the wrong package? --- 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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r134006399
--- 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 --
Does it make sense to possibly pull out all the constants into a location
that can be shared without cross-pollinating or referencing classes that aren't
related
---
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 kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/716#discussion_r134007584 --- 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 -- Is this class in the wrong package? --- 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 kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/716#discussion_r134053987 --- 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 -- What does an ExecutionContext have to do with socket? --- 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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/719#discussion_r134050811
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java
---
@@ -40,20 +42,28 @@ public void receiveMessage(InputStream inputStream,
OutputStream outputStream,
properties.setProperty("username",
authenticationRequest.getUsername());
properties.setProperty("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;
+return authorizer != null;
--- End diff --
I must disagree with this logic. Something is NOT `authenticated` just
because there is an authorizer populated. A authorizer should be constructed
BECAUSE something was `authenticated`
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/719#discussion_r134051130
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java
---
@@ -40,20 +42,28 @@ public void receiveMessage(InputStream inputStream,
OutputStream outputStream,
properties.setProperty("username",
authenticationRequest.getUsername());
properties.setProperty("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);
--- End diff --
Why do we need a `ProtobufSimpleAuthorizer` per principal? Could we not
have a single `Authorizer` that will authenticate all principals?
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/719#discussion_r134025383
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthorizer.java
---
@@ -0,0 +1,42 @@
+/*
+ * 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;
+
+import java.io.EOFException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.util.Properties;
+
+import org.apache.geode.security.AuthenticationFailedException;
+import org.apache.geode.security.ResourcePermission;
+import org.apache.geode.security.SecurityManager;
+import org.apache.geode.security.StreamAuthenticator;
+import org.apache.geode.security.StreamAuthorizer;
+
+public class ProtobufSimpleAuthorizer implements StreamAuthorizer {
+ private final Object authenticatedPrincipal;
+ private final SecurityManager securityManager;
+
+ public ProtobufSimpleAuthorizer(Object authenticatedPrincipal,
SecurityManager securityManager) {
+this.authenticatedPrincipal = authenticatedPrincipal;
+this.securityManager = securityManager;
+ }
+
+ @Override
+ public boolean authorize(ResourcePermission permissionRequested) {
--- End diff --
why would I not pass in the Principal and requested permissions? Then I can
have 1 authorizer and not 1 per user.
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/702#discussion_r134328545
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/net/SocketCloser.java ---
@@ -96,46 +99,55 @@ 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.
+ */
+ @Deprecated
+ private void createThreadPool() {
+ExecutorService executorService;
+final ThreadGroup threadGroup =
+LoggingThreadGroup.createThreadGroup("Socket asyncClose", logger);
+ThreadFactory threadFactory = new ThreadFactory() {
+ public Thread newThread(final Runnable command) {
+Thread thread = new Thread(threadGroup, command);
+thread.setDaemon(true);
+return thread;
+ }
+};
+
+executorService = new ThreadPoolExecutor(asyncClosePoolMaxThreads,
asyncClosePoolMaxThreads,
+asyncCloseWaitTime, asyncCloseWaitUnits, new
LinkedBlockingQueue<>(), threadFactory);
}
/**
* Call this method if you know all the resources in the closer for the
given address are no
* longer needed. Currently a thread pool is kept for each address and
if you know that an address
* no longer needs its pool then you should call this method.
*/
- public void releaseResourcesForAddress(String address) {
-synchronized (asyncCloseExecutors) {
- ThreadPoolExecutor pool = asyncCloseExecutors.get(address);
- if (pool != null) {
-pool.shutdown();
-asyncCloseExecutors.remove(address);
- }
-}
- }
- private boolean isClosed() {
-synchronized (asyncCloseExecutors) {
- return this.closed;
+ public void releaseResourcesForAddress(String address) {
+ExecutorService executorService = asyncCloseExecutors.remove(address);
+if (executorService != null) {
+ executorService.shutdown();
--- End diff --
@bschuchardt in behavior this code is no different than what the original
implementation was. The only difference is that the synchronization block's
scope has been reduced. In addition to that, the thread pool has been replaced
with newWorkStealingPool. This pool is a little more optimal than the original
implementation and can reduce the initial startup requirements of a
ThreadPoolExecutor.
---
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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/716#discussion_r134514607
--- 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 think the chances of the Locator changing on us would be really hard, as
this code is running on a locator.
---
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 #736: GEODE-3503: Removal of Codec classes left behind.
GitHub user kohlmu-pivotal opened a pull request: https://github.com/apache/geode/pull/736 GEODE-3503: Removal of Codec classes left behind. Added tests to test the remaining JSONCodec. Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has 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? - [ ] 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/apache/geode feature/GEODE-3503 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/736.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 #736 commit 6c807e8267b6ac6878e4b497d0b3d680ea496ef1 Author: Udo Kohlmeyer Date: 2017-08-23T20:48:11Z GEODE-3503: Removal of Codec classes left behind. Added tests to test the remaining JSONCodec. --- 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 #736: GEODE-3503: Removal of Codec classes left behind.
Github user kohlmu-pivotal closed the pull request at: https://github.com/apache/geode/pull/736 --- 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 kohlmu-pivotal opened a pull request: https://github.com/apache/geode/pull/737 GEODE-3503: Removal of Codec classes left behind. Added tests to test the remaining JSONCodec. Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has 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? - [ ] 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/apache/geode feature/GEODE-3503 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/737.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 #737 commit a182a5a956d8a2e299fa3fb1307a79aa7c353e9e Author: Udo Kohlmeyer Date: 2017-08-23T20:48:11Z GEODE-3503: Removal of Codec classes left behind. Added tests to test the remaining JSONCodec. --- 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 kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/737#discussion_r134895876
--- Diff: geode-protobuf/src/main/proto/region_API.proto ---
@@ -58,6 +58,7 @@ message GetAllRequest {
message GetAllResponse {
repeated Entry entries = 1;
+repeated KeyedErrorResponse failedKeys = 2;
--- End diff --
reverted
---
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 #739: GEODE-3385: Change GetAllRequest to return list of ...
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/739#discussion_r135055386
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
---
@@ -50,26 +53,52 @@
.makeErrorResponse(ProtocolErrorCode.REGION_NOT_FOUND.codeValue,
"Region not found"));
}
-try {
- Set keys = new HashSet<>();
- for (BasicTypes.EncodedValue key : request.getKeyList()) {
-keys.add(ProtobufUtilities.decodeValue(serializationService, key));
- }
- Map results = region.getAll(keys);
- Set entries = new HashSet<>();
- for (Map.Entry entry : results.entrySet()) {
-entries.add(
-ProtobufUtilities.createEntry(serializationService,
entry.getKey(), entry.getValue()));
- }
- return
Success.of(RegionAPI.GetAllResponse.newBuilder().addAllEntries(entries).build());
-} catch (UnsupportedEncodingTypeException ex) {
- return Failure.of(ProtobufResponseUtilities.makeErrorResponse(
- ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue, "Encoding not
supported."));
-} catch (CodecNotRegisteredForTypeException ex) {
- return Failure.of(ProtobufResponseUtilities.makeErrorResponse(
- ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue,
- "Codec error in protobuf deserialization."));
+Map> resultsCollection =
request.getKeyList().stream()
+.map((key) -> processOneMessage(serializationService, region, key))
+.collect(Collectors.partitioningBy(x -> x instanceof
BasicTypes.Entry));
+RegionAPI.GetAllResponse.Builder responseBuilder =
RegionAPI.GetAllResponse.newBuilder();
+
+for (Object entry : resultsCollection.get(true)) {
+ responseBuilder.addEntries((BasicTypes.Entry) entry);
+}
+
+for (Object entry : resultsCollection.get(false)) {
+ responseBuilder.addFailures((BasicTypes.KeyedError) entry);
}
+
+return Success.of(responseBuilder.build());
}
+ private Object processOneMessage(SerializationService
serializationService, Region region,
+ BasicTypes.EncodedValue key) {
+try {
+ Object decodedKey =
ProtobufUtilities.decodeValue(serializationService, key);
+ Object value = region.get(decodedKey);
+ return ProtobufUtilities.createEntry(serializationService,
decodedKey, value);
+} catch (CodecNotRegisteredForTypeException |
UnsupportedEncodingTypeException ex) {
+ return BasicTypes.KeyedError.newBuilder().setKey(key)
+ .setError(BasicTypes.Error.newBuilder()
+
.setErrorCode(ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue)
+ .setMessage("Encoding not supported."))
+ .build();
+} catch (org.apache.geode.distributed.LeaseExpiredException |
TimeoutException e) {
+ return BasicTypes.KeyedError.newBuilder().setKey(key)
+ .setError(BasicTypes.Error.newBuilder()
+ .setErrorCode(ProtocolErrorCode.OPERATION_TIMEOUT.codeValue)
+ .setMessage("Operation timed out: " + e.getMessage()))
+ .build();
+} catch (CacheLoaderException | PartitionedRegionStorageException e) {
+ return BasicTypes.KeyedError.newBuilder().setKey(key)
+ .setError(BasicTypes.Error.newBuilder()
+ .setErrorCode(ProtocolErrorCode.DATA_UNREACHABLE.codeValue)
+ .setMessage("Data unreachable: " + e.getMessage()))
+ .build();
+} catch (NullPointerException | IllegalArgumentException e) {
+ return BasicTypes.KeyedError.newBuilder().setKey(key)
+ .setError(BasicTypes.Error.newBuilder()
+
.setErrorCode(ProtocolErrorCode.CONSTRAINT_VIOLATION.codeValue)
+ .setMessage("Invalid input: " + e.getMessage()))
+ .build();
--- End diff --
Do we have any tests that prove that these exceptions are thrown and
correctly handled?
---
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 #739: GEODE-3385: Change GetAllRequest to return list of ...
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/739#discussion_r135055785
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
---
@@ -50,26 +53,52 @@
.makeErrorResponse(ProtocolErrorCode.REGION_NOT_FOUND.codeValue,
"Region not found"));
}
-try {
- Set keys = new HashSet<>();
- for (BasicTypes.EncodedValue key : request.getKeyList()) {
-keys.add(ProtobufUtilities.decodeValue(serializationService, key));
- }
- Map results = region.getAll(keys);
- Set entries = new HashSet<>();
- for (Map.Entry entry : results.entrySet()) {
-entries.add(
-ProtobufUtilities.createEntry(serializationService,
entry.getKey(), entry.getValue()));
- }
- return
Success.of(RegionAPI.GetAllResponse.newBuilder().addAllEntries(entries).build());
-} catch (UnsupportedEncodingTypeException ex) {
- return Failure.of(ProtobufResponseUtilities.makeErrorResponse(
- ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue, "Encoding not
supported."));
-} catch (CodecNotRegisteredForTypeException ex) {
- return Failure.of(ProtobufResponseUtilities.makeErrorResponse(
- ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue,
- "Codec error in protobuf deserialization."));
+Map> resultsCollection =
request.getKeyList().stream()
+.map((key) -> processOneMessage(serializationService, region, key))
+.collect(Collectors.partitioningBy(x -> x instanceof
BasicTypes.Entry));
+RegionAPI.GetAllResponse.Builder responseBuilder =
RegionAPI.GetAllResponse.newBuilder();
+
+for (Object entry : resultsCollection.get(true)) {
+ responseBuilder.addEntries((BasicTypes.Entry) entry);
+}
+
+for (Object entry : resultsCollection.get(false)) {
+ responseBuilder.addFailures((BasicTypes.KeyedError) entry);
}
+
+return Success.of(responseBuilder.build());
}
+ private Object processOneMessage(SerializationService
serializationService, Region region,
+ BasicTypes.EncodedValue key) {
+try {
+ Object decodedKey =
ProtobufUtilities.decodeValue(serializationService, key);
+ Object value = region.get(decodedKey);
+ return ProtobufUtilities.createEntry(serializationService,
decodedKey, value);
+} catch (CodecNotRegisteredForTypeException |
UnsupportedEncodingTypeException ex) {
+ return BasicTypes.KeyedError.newBuilder().setKey(key)
+ .setError(BasicTypes.Error.newBuilder()
+
.setErrorCode(ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue)
+ .setMessage("Encoding not supported."))
+ .build();
+} catch (org.apache.geode.distributed.LeaseExpiredException |
TimeoutException e) {
+ return BasicTypes.KeyedError.newBuilder().setKey(key)
+ .setError(BasicTypes.Error.newBuilder()
+ .setErrorCode(ProtocolErrorCode.OPERATION_TIMEOUT.codeValue)
+ .setMessage("Operation timed out: " + e.getMessage()))
+ .build();
+} catch (CacheLoaderException | PartitionedRegionStorageException e) {
+ return BasicTypes.KeyedError.newBuilder().setKey(key)
+ .setError(BasicTypes.Error.newBuilder()
+ .setErrorCode(ProtocolErrorCode.DATA_UNREACHABLE.codeValue)
+ .setMessage("Data unreachable: " + e.getMessage()))
+ .build();
+} catch (NullPointerException | IllegalArgumentException e) {
+ return BasicTypes.KeyedError.newBuilder().setKey(key)
+ .setError(BasicTypes.Error.newBuilder()
+
.setErrorCode(ProtocolErrorCode.CONSTRAINT_VIOLATION.codeValue)
+ .setMessage("Invalid input: " + e.getMessage()))
+ .build();
--- End diff --
Also, it would be A LOT nicer to have some utility method that states
`createErrorMessage(code,description)`
---
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 #742: GEODE-3473: Initial commit of the internal package ...
GitHub user kohlmu-pivotal opened a pull request: https://github.com/apache/geode/pull/742 GEODE-3473: Initial commit of the internal package renaming refactor. Changing the protobuf file refactor first. Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has 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? - [ ] 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/apache/geode feature/GEODE-3473 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/742.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 #742 commit acc9cebcb996da8413e9bfd47cc0bf20976ebf23 Author: Udo Kohlmeyer Date: 2017-08-24T19:54:38Z GEODE-3473: Initial commit of the internal package renaming refactor. Changing the protobuf file refactor first. --- 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 kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/742 @galen-pivotal @bschuchardt @pivotal-amurmann @WireBaron @hiteshk25 --- 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 kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/742 @pivotal-amurmann no files should have been moved. Only class import locations have changed. --- 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 #245: native-client-software-grant - ClientMetadata::getServerLo...
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/245 @doribd @fdaniel7 could you please close this PR if it is not required anymore. --- 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 #325: merge new version
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/325 @zmyer could you please confirm that this PR is still relevant. If not I will close it CoB tomorrow ( 25 Aug) --- 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 #739: GEODE-3385: Change GetAllRequest to return list of ...
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/739#discussion_r135899858
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
---
@@ -50,26 +53,52 @@
.makeErrorResponse(ProtocolErrorCode.REGION_NOT_FOUND.codeValue,
"Region not found"));
}
-try {
- Set keys = new HashSet<>();
- for (BasicTypes.EncodedValue key : request.getKeyList()) {
-keys.add(ProtobufUtilities.decodeValue(serializationService, key));
- }
- Map results = region.getAll(keys);
- Set entries = new HashSet<>();
- for (Map.Entry entry : results.entrySet()) {
-entries.add(
-ProtobufUtilities.createEntry(serializationService,
entry.getKey(), entry.getValue()));
- }
- return
Success.of(RegionAPI.GetAllResponse.newBuilder().addAllEntries(entries).build());
-} catch (UnsupportedEncodingTypeException ex) {
- return Failure.of(ProtobufResponseUtilities.makeErrorResponse(
- ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue, "Encoding not
supported."));
-} catch (CodecNotRegisteredForTypeException ex) {
- return Failure.of(ProtobufResponseUtilities.makeErrorResponse(
- ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue,
- "Codec error in protobuf deserialization."));
+Map> resultsCollection =
request.getKeyList().stream()
+.map((key) -> processOneMessage(serializationService, region, key))
+.collect(Collectors.partitioningBy(x -> x instanceof
BasicTypes.Entry));
+RegionAPI.GetAllResponse.Builder responseBuilder =
RegionAPI.GetAllResponse.newBuilder();
+
+for (Object entry : resultsCollection.get(true)) {
+ responseBuilder.addEntries((BasicTypes.Entry) entry);
+}
+
+for (Object entry : resultsCollection.get(false)) {
+ responseBuilder.addFailures((BasicTypes.KeyedError) entry);
}
+
+return Success.of(responseBuilder.build());
}
+ private Object processOneMessage(SerializationService
serializationService, Region region,
+ BasicTypes.EncodedValue key) {
+try {
+ Object decodedKey =
ProtobufUtilities.decodeValue(serializationService, key);
+ Object value = region.get(decodedKey);
--- End diff --
I agree with you that getAll should be used, but if the API does not
provide the functionality that this interface is to expose, then get is a
viable option until the API issues are addressed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #404: Geode 2469
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/404 I like the fact that we are now dealing with collections on a single machine, rather than all the elements spread distributed. Would you possibly have some perf metrics in relation to larger collections. e.g 1mil entries vs 100. I imagine that as the number of entries in the collections grow, so will the insert performance. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #450: GEODE-2632: create ClientCachePutBench
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/450#discussion_r111093862
--- Diff:
geode-core/src/jmh/java/org/apache/geode/internal/cache/tier/sockets/command/ClientCachePutBench.java
---
@@ -0,0 +1,199 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
ANY KIND, either express
+ * or implied. See the License for the specific language governing
permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.tier.sockets.command;
+
+import static java.lang.System.*;
+import static java.util.concurrent.TimeUnit.*;
+import static org.apache.commons.io.FileUtils.*;
+import static org.apache.commons.lang.StringUtils.*;
+import static org.apache.geode.cache.client.ClientRegionShortcut.*;
+import static org.apache.geode.distributed.AbstractLauncher.Status.*;
+import static org.apache.geode.distributed.ConfigurationProperties.*;
+import static org.apache.geode.distributed.internal.DistributionConfig.*;
+import static org.apache.geode.internal.AvailablePort.*;
+import static org.apache.geode.test.dunit.NetworkUtils.*;
+import static org.assertj.core.api.Assertions.*;
+import static org.awaitility.Awaitility.*;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.distributed.ServerLauncher;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.junit.rules.TemporaryFolder;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Warmup;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URL;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Benchmark that measures throughput of client performing puts to a loner
server.
+ */
+@Measurement(iterations = 3, time = 3, timeUnit = MINUTES)
+@Warmup(iterations = 3, time = 1, timeUnit = MINUTES)
+@Fork(3)
+@BenchmarkMode(Mode.Throughput)
+@OutputTimeUnit(TimeUnit.SECONDS)
+@State(Scope.Thread)
+@SuppressWarnings("unused")
+public class ClientCachePutBench {
+
+ static final long PROCESS_READER_TIMEOUT = 60 * 1000;
+ static final String CLASS_NAME =
ClientCachePutBench.class.getSimpleName();
+ static final String PACKAGE_NAME =
+ replace(ClientCachePutBench.class.getPackage().getName(), ".", "/");
+ static final String REGION_NAME = CLASS_NAME + "-region";
+ static final String SERVER_XML_NAME = "/" + PACKAGE_NAME + "/" +
CLASS_NAME + "-server.xml";
+
+ @State(Scope.Benchmark)
+ public static class ClientState {
+
+Random random;
+Region region;
+
+private Process process;
+private volatile ProcessStreamReader processOutReader;
+private volatile ProcessStreamReader processErrReader;
+
+private int serverPort;
+private ServerLauncher launcher;
+private File serverDirectory;
+private ClientCache clientCache;
+
+private TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+@Setup(Level.Trial)
+public void startServer() throws Exception {
+ System.out.println("\n" + "[ClientCachePutBench] startServer");
+
+ this.random = new Random(nanoTime());
+
+ this.temporaryFolder.create();
+ this.serverDirectory = this.temporaryFolder.getRoot();
+
+ startServerProcess(
[GitHub] geode pull request #462: GEODE-2103 Update gfsh start server|locator command
Github user kohlmu-pivotal commented on a diff in the pull request: https://github.com/apache/geode/pull/462#discussion_r112304411 --- Diff: geode-docs/tools_modules/gfsh/command-pages/start.html.md.erb --- @@ -398,6 +400,17 @@ See Overview of cluster-config +\-\-http-service-port +Specifies the port on which the HTTP service will listen. --- End diff -- Maybe this should be "Specifies the port on which the HTTP service is started 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 issue #305: [GEODE-1923] Fix a test race condition.
Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/305 I doubt that FixedPRSinglehopDUnitTest::primaryBucketsOnServer provides any benefit over FixedPRSinglehopDUnitTest.primaryBucketsOnServer() --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #313: [GEODE-1407] Change a FlakyTest to distributedTest.
Github user kohlmu-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/313#discussion_r94819019
--- Diff:
geode-core/src/test/java/org/apache/geode/cache30/ReconnectDUnitTest.java ---
@@ -534,41 +532,35 @@ public Object call() throws CacheException {
Cache cache = getCache();
Region myRegion = cache.getRegion("root/myRegion");
myRegion.put("MyKey1", "MyValue1");
-// myRegion.put("Mykey2", "MyValue2");
return savedSystem.getDistributedMember();
}
};
vm1.invoke(create1);
-
try {
-
dm = getDMID(vm0);
createGfshWaitingThread(vm0);
forceDisconnect(vm0);
newdm = waitForReconnect(vm0);
assertGfshWaitingThreadAlive(vm0);
- vm0.invoke(new SerializableRunnable("check for running locator") {
-public void run() {
- WaitCriterion wc = new WaitCriterion() {
-public boolean done() {
- return Locator.getLocator() != null;
-}
-
-public String description() {
- return "waiting for locator to restart";
-}
- };
- Wait.waitForCriterion(wc, 3, 1000, false);
+ boolean running = (Boolean) vm0.invoke(new
SerializableCallable("check for running locator") {
--- End diff --
This can be written as a "proper" Lambda if you wanted to continue the
refactor. vm0.invoke("check for running locator", () -> {...});
---
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.
---
