[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[ https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16084908#comment-16084908 ] ASF GitHub Bot commented on GEODE-2997: --- GitHub user WireBaron opened a pull request: https://github.com/apache/geode/pull/629 GEODE-2997: New flow getAll/putAll Changed get response to indicate if LookupFailure was a missing key or key with null value, added test Added GetAllRequestOperationHandler and unit test Added PutAllRequestOperationHandler and unit test Added an integration test covering the putAll and getAll operations Removed serverInternal and retriable fields from ErrorResponse, replaced with errorCode field Signed-off-by: Brian Rowe Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x] Is your initial contribution a single, squashed commit? - [x] Does `gradlew build` run cleanly? - [x] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### 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 d...@geode.apache.org. You can merge this pull request into a Git repository by running: $ git pull https://github.com/WireBaron/geode feature/GEODE-2997 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/629.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 #629 commit 510d579788abdd04e49b6ee7748fe0953e972e99 Author: Alexander Murmann Date: 2017-07-06T00:36:20Z GEODE-2997: New flow getAll/putAll Changed get response to indicate if LookupFailure was a missing key or key with null value, added test Added GetAllRequestOperationHandler and unit test Added PutAllRequestOperationHandler and unit test Added an integration test covering the putAll and getAll operations Removed serverInternal and retriable fields from ErrorResponse, replaced with errorCode field Signed-off-by: Brian Rowe > New flow: putAll/getAll > --- > > Key: GEODE-2997 > URL: https://issues.apache.org/jira/browse/GEODE-2997 > Project: Geode > Issue Type: Sub-task > Components: client/server >Reporter: Brian Baynes > > Create proto message definitions and op handler for putAll/getAll messages. > Client should be able to complete putAll/getAll of primitives and JSON with > response. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[ https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16084940#comment-16084940 ] ASF GitHub Bot commented on GEODE-2997: --- Github user asfgit closed the pull request at: https://github.com/apache/geode/pull/629 > New flow: putAll/getAll > --- > > Key: GEODE-2997 > URL: https://issues.apache.org/jira/browse/GEODE-2997 > Project: Geode > Issue Type: Sub-task > Components: client/server >Reporter: Brian Baynes > > Create proto message definitions and op handler for putAll/getAll messages. > Client should be able to complete putAll/getAll of primitives and JSON with > response. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[ https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16084939#comment-16084939 ] ASF subversion and git services commented on GEODE-2997: Commit cd1a3235761610a5454e215f6ceb096e6fd6df77 in geode's branch refs/heads/develop from [~amurmann] [ https://git-wip-us.apache.org/repos/asf?p=geode.git;h=cd1a323 ] GEODE-2997: New flow getAll/putAll. This now closes #629 > New flow: putAll/getAll > --- > > Key: GEODE-2997 > URL: https://issues.apache.org/jira/browse/GEODE-2997 > Project: Geode > Issue Type: Sub-task > Components: client/server >Reporter: Brian Baynes > > Create proto message definitions and op handler for putAll/getAll messages. > Client should be able to complete putAll/getAll of primitives and JSON with > response. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[ https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16095444#comment-16095444 ] ASF GitHub Bot commented on GEODE-2997: --- GitHub user galen-pivotal opened a pull request: https://github.com/apache/geode/pull/649 GEODE-2997: Change new protocol GetAllResponse. @kohlmu-pivotal @hiteshk25 @WireBaron @pivotal-amurmann @bschuchardt I think it's better to return the errors and the successful keys so that one failed lookup doesn't mess up the whole request. Imagining this as a conversational PR to see what the response is. Signed-off-by: Galen O'Sullivan 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? - [NO.] 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 d...@geode.apache.org. You can merge this pull request into a Git repository by running: $ git pull https://github.com/galen-pivotal/geode changeGetAllAPI Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/649.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 #649 commit 2b07dfc3cae729b8e23655fa11f3e8d485c520c4 Author: Alexander Murmann Date: 2017-07-20T21:44:16Z GEODE-2997: Change new protocol GetAllResponse. I think it's better to return the errors and the successful keys so that one failed lookup doesn't mess up the whole request. Imagining this as a conversational PR to see what the response is. Signed-off-by: Galen O'Sullivan > New flow: putAll/getAll > --- > > Key: GEODE-2997 > URL: https://issues.apache.org/jira/browse/GEODE-2997 > Project: Geode > Issue Type: Sub-task > Components: client/server >Reporter: Brian Baynes > > Create proto message definitions and op handler for putAll/getAll messages. > Client should be able to complete putAll/getAll of primitives and JSON with > response. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[ https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16095491#comment-16095491 ] ASF GitHub Bot commented on GEODE-2997: --- Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/649 Some other thoughts from today: * make GetAllResponse return a collection of Entries and a collection of errors. * make Get and GetAll return the same type of response, as mentioned above. Get will always return a single key in either the success or error collections. - maybe looking in a collection is extra overhead for a simple Get request? - bigger errors (like malformed messages) will still return errors at the Response level rather than errors nested in the response message. Thoughts? > New flow: putAll/getAll > --- > > Key: GEODE-2997 > URL: https://issues.apache.org/jira/browse/GEODE-2997 > Project: Geode > Issue Type: Sub-task > Components: client/server >Reporter: Brian Baynes > > Create proto message definitions and op handler for putAll/getAll messages. > Client should be able to complete putAll/getAll of primitives and JSON with > response. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[
https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16095540#comment-16095540
]
ASF GitHub Bot commented on GEODE-2997:
---
Github user WireBaron commented on a diff in the pull request:
https://github.com/apache/geode/pull/649#discussion_r128652774
--- 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 {
--- End diff --
These are the same fields as ErrorResponse and KeyedErrorResponse in the
outstanding PutAll pull request, can you comment in that PR to change the names
if you prefer these?
> New flow: putAll/getAll
> ---
>
> Key: GEODE-2997
> URL: https://issues.apache.org/jira/browse/GEODE-2997
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Brian Baynes
>
> Create proto message definitions and op handler for putAll/getAll messages.
> Client should be able to complete putAll/getAll of primitives and JSON with
> response.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[
https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16096763#comment-16096763
]
ASF GitHub Bot commented on GEODE-2997:
---
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.
> New flow: putAll/getAll
> ---
>
> Key: GEODE-2997
> URL: https://issues.apache.org/jira/browse/GEODE-2997
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Brian Baynes
>
> Create proto message definitions and op handler for putAll/getAll messages.
> Client should be able to complete putAll/getAll of primitives and JSON with
> response.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[
https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16096764#comment-16096764
]
ASF GitHub Bot commented on GEODE-2997:
---
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`
> New flow: putAll/getAll
> ---
>
> Key: GEODE-2997
> URL: https://issues.apache.org/jira/browse/GEODE-2997
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Brian Baynes
>
> Create proto message definitions and op handler for putAll/getAll messages.
> Client should be able to complete putAll/getAll of primitives and JSON with
> response.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[
https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16101891#comment-16101891
]
ASF GitHub Bot commented on GEODE-2997:
---
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/649#discussion_r129626228
--- 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 --
The reasoning is, we're not just using it as an ErrorResponse at the same
level as a PutResponse or GetResponse -- it gets embedded in other responses
now.
> New flow: putAll/getAll
> ---
>
> Key: GEODE-2997
> URL: https://issues.apache.org/jira/browse/GEODE-2997
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Brian Baynes
>
> Create proto message definitions and op handler for putAll/getAll messages.
> Client should be able to complete putAll/getAll of primitives and JSON with
> response.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[
https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16101896#comment-16101896
]
ASF GitHub Bot commented on GEODE-2997:
---
Github user galen-pivotal commented on a diff in the pull request:
https://github.com/apache/geode/pull/649#discussion_r129626464
--- 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 --
That's fine too. I don't have a real strong preference.
> New flow: putAll/getAll
> ---
>
> Key: GEODE-2997
> URL: https://issues.apache.org/jira/browse/GEODE-2997
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Brian Baynes
>
> Create proto message definitions and op handler for putAll/getAll messages.
> Client should be able to complete putAll/getAll of primitives and JSON with
> response.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[
https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16101912#comment-16101912
]
ASF GitHub Bot commented on GEODE-2997:
---
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
> New flow: putAll/getAll
> ---
>
> Key: GEODE-2997
> URL: https://issues.apache.org/jira/browse/GEODE-2997
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Brian Baynes
>
> Create proto message definitions and op handler for putAll/getAll messages.
> Client should be able to complete putAll/getAll of primitives and JSON with
> response.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[ https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16103852#comment-16103852 ] ASF GitHub Bot commented on GEODE-2997: --- Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/649 This isn't code complete; I'm planning to work on this when I have the time (unless anyone else wants to). > New flow: putAll/getAll > --- > > Key: GEODE-2997 > URL: https://issues.apache.org/jira/browse/GEODE-2997 > Project: Geode > Issue Type: Sub-task > Components: client/server >Reporter: Brian Baynes > > Create proto message definitions and op handler for putAll/getAll messages. > Client should be able to complete putAll/getAll of primitives and JSON with > response. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[ https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16127437#comment-16127437 ] ASF GitHub Bot commented on GEODE-2997: --- Github user kohlmu-pivotal commented on the issue: https://github.com/apache/geode/pull/649 @galen-pivotal is this PR active anymore? > New flow: putAll/getAll > --- > > Key: GEODE-2997 > URL: https://issues.apache.org/jira/browse/GEODE-2997 > Project: Geode > Issue Type: Sub-task > Components: client/server >Reporter: Brian Baynes > > Create proto message definitions and op handler for putAll/getAll messages. > Client should be able to complete putAll/getAll of primitives and JSON with > response. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[ https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16139280#comment-16139280 ] ASF GitHub Bot commented on GEODE-2997: --- Github user galen-pivotal closed the pull request at: https://github.com/apache/geode/pull/649 > New flow: putAll/getAll > --- > > Key: GEODE-2997 > URL: https://issues.apache.org/jira/browse/GEODE-2997 > Project: Geode > Issue Type: Sub-task > Components: client/server >Reporter: Brian Baynes >Assignee: Udo Kohlmeyer > > Create proto message definitions and op handler for putAll/getAll messages. > Client should be able to complete putAll/getAll of primitives and JSON with > response. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (GEODE-2997) New flow: putAll/getAll
[ https://issues.apache.org/jira/browse/GEODE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16139279#comment-16139279 ] ASF GitHub Bot commented on GEODE-2997: --- Github user galen-pivotal commented on the issue: https://github.com/apache/geode/pull/649 closing for #739 . > New flow: putAll/getAll > --- > > Key: GEODE-2997 > URL: https://issues.apache.org/jira/browse/GEODE-2997 > Project: Geode > Issue Type: Sub-task > Components: client/server >Reporter: Brian Baynes >Assignee: Udo Kohlmeyer > > Create proto message definitions and op handler for putAll/getAll messages. > Client should be able to complete putAll/getAll of primitives and JSON with > response. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
