[GitHub] geode pull request #630: GEODE-3141: GetRegion Operation implemented

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/geode/pull/630


---
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

[kohlmu-pivotal]

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 #630: GEODE-3141: GetRegion Operation implemented

[kohlmu-pivotal]

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

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127756778
  
--- 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 --

Did there used to be a success variable here?


---
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

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127299289
  
--- 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 --

spotlessApply doesn't seem to care about the import order, however IntelliJ 
will reorder according the style guide every time you run "optimize imports".  
Patrick was flagging every pull request with the outdated order for a while.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #630: GEODE-3141: GetRegion Operation implemented

[kohlmu-pivotal]

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

[kohlmu-pivotal]

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

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127292855
  
--- 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 --

Have you updated your IntelliJ style guide file with the June 13 version?  
They've modified the order of imports somewhat.


---
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

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127290974
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
 ---
@@ -278,6 +278,31 @@ public void 
useSSL_testNewProtocolHeaderLeadsToNewProtocolServerConnection() thr
 testNewProtocolHeaderLeadsToNewProtocolServerConnection();
   }
 
+  @Test
+  public void testNewProtocolGetRegionCallSucceeds() throws Exception {
+System.setProperty("geode.feature-protobuf-protocol", "true");
+
+Socket socket = new Socket("localhost", cacheServerPort);
+Awaitility.await().atMost(5, 
TimeUnit.SECONDS).until(socket::isConnected);
+OutputStream outputStream = socket.getOutputStream();
+outputStream.write(110);
+
+
+ProtobufProtocolSerializer protobufProtocolSerializer = new 
ProtobufProtocolSerializer();
+ClientProtocol.Message getRegionMessage =
+MessageUtil.makeGetRegionRequestMessage(TEST_REGION, 
ClientProtocol.MessageHeader.newBuilder().build());
+protobufProtocolSerializer.serialize(getRegionMessage, outputStream);
+
+ClientProtocol.Message message =
+protobufProtocolSerializer.deserialize(socket.getInputStream());
+assertEquals(ClientProtocol.Message.MessageTypeCase.RESPONSE, 
message.getMessageTypeCase());
+ClientProtocol.Response response = message.getResponse();
+assertEquals(ClientProtocol.Response.ResponseAPICase.GETREGIONRESPONSE,
+response.getResponseAPICase());
+response.getGetRegionResponse();
+//TODO add some assertions for Region data
--- End diff --

Can we resolve this TODO before pushing 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

[kohlmu-pivotal]

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

[kohlmu-pivotal]

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

[kohlmu-pivotal]

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

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127285451
  
--- 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 --

Does this pass spotless?  I thought all of these wonky indentations were 
actually introduced by spotlessApply?


---
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

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127285154
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
 ---
@@ -278,6 +278,31 @@ public void 
useSSL_testNewProtocolHeaderLeadsToNewProtocolServerConnection() thr
 testNewProtocolHeaderLeadsToNewProtocolServerConnection();
   }
 
+  @Test
+  public void testNewProtocolGetRegionCallSucceeds() throws Exception {
+System.setProperty("geode.feature-protobuf-protocol", "true");
+
+Socket socket = new Socket("localhost", cacheServerPort);
+Awaitility.await().atMost(5, 
TimeUnit.SECONDS).until(socket::isConnected);
+OutputStream outputStream = socket.getOutputStream();
+outputStream.write(110);
+
+
+ProtobufProtocolSerializer protobufProtocolSerializer = new 
ProtobufProtocolSerializer();
+ClientProtocol.Message getRegionMessage =
+MessageUtil.makeGetRegionRequestMessage(TEST_REGION, 
ClientProtocol.MessageHeader.newBuilder().build());
+protobufProtocolSerializer.serialize(getRegionMessage, outputStream);
+
+ClientProtocol.Message message =
+protobufProtocolSerializer.deserialize(socket.getInputStream());
+assertEquals(ClientProtocol.Message.MessageTypeCase.RESPONSE, 
message.getMessageTypeCase());
+ClientProtocol.Response response = message.getResponse();
+assertEquals(ClientProtocol.Response.ResponseAPICase.GETREGIONRESPONSE,
+response.getResponseAPICase());
+response.getGetRegionResponse();
+//TODO add some assertions for Region data
--- End diff --

Can we either add the assertions or put a chore in the backlog to backfill 
this?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #630: GEODE-3141: GetRegion Operation implemented

[bschuchardt]

Github user bschuchardt commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127276853
  
--- 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 --

While I don't really like the name "dataPolicy" it seems like renaming it 
could cause confusion.  Geode docs don't mention a Region "type".


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #630: GEODE-3141: GetRegion Operation implemented

[kohlmu-pivotal]

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.
---