You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by kohlmu-pivotal <gi...@git.apache.org> on 2017/07/13 00:27:06 UTC

[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 <uk...@pivotal.io>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by galen-pivotal <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by WireBaron <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by WireBaron <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kohlmu-pivotal <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kohlmu-pivotal <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kohlmu-pivotal <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by WireBaron <gi...@git.apache.org>.
Github user WireBaron commented on a diff in the pull request:

    https://github.com/apache/geode/pull/630#discussion_r127289766
  
    --- 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 --
    
    For when we don't have a key constaint, will protoRegionBuilder.setKeyConstraint("") do the same thing as simply not setting this field?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kohlmu-pivotal <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kohlmu-pivotal <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by WireBaron <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kohlmu-pivotal <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by bschuchardt <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by galen-pivotal <gi...@git.apache.org>.
Github user galen-pivotal commented on the issue:

    https://github.com/apache/geode/pull/630
  
    Looks like Spotless is failing in CI.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kohlmu-pivotal <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pivotal-amurmann <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kohlmu-pivotal <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by galen-pivotal <gi...@git.apache.org>.
Github user galen-pivotal commented on a diff in the pull request:

    https://github.com/apache/geode/pull/630#discussion_r127820947
  
    --- Diff: geode-protobuf/src/main/proto/basicTypes.proto ---
    @@ -52,7 +52,12 @@ message CallbackArguments {
     
     message Region {
    --- End diff --
    
    Why not inline the contents of this in `GetRegionResponse`?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by WireBaron <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---