[jira] [Commented] (GEODE-3386) Create Error type for KeyedErrorResponse and ErrorResponse
[
https://issues.apache.org/jira/browse/GEODE-3386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16127432#comment-16127432
]
ASF GitHub Bot commented on GEODE-3386:
---
Github user asfgit closed the pull request at:
https://github.com/apache/geode/pull/700
> Create Error type for KeyedErrorResponse and ErrorResponse
> --
>
> Key: GEODE-3386
> URL: https://issues.apache.org/jira/browse/GEODE-3386
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Galen O'Sullivan
>
> For logical separation of the new client API, it will be better to have an
> Error that is contained by ErrorResponse, rather than having
> KeyedErrorResponse contain an ErrorResponse.
> In pseudo-protobuf,
> {code}
> PutAllResponse {
> repeated Entry successes = 1,
> repeated KeyedErrorResponse errors = 2,
> }
> KeyedErrorResponse {
> Key,
> ErrorResponse
> }
> ErrorResponse {
> string
> }
> {code}
> instead,
> {code}
> KeyedErrorResponse {
> Key,
> Error,
> }
> {code}
> and
> {code}
> ErrorResponse {
> Error
> }
> Error {
> string
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-3386) Create Error type for KeyedErrorResponse and ErrorResponse
[
https://issues.apache.org/jira/browse/GEODE-3386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16127431#comment-16127431
]
ASF subversion and git services commented on GEODE-3386:
Commit a3c0ebaf0d0246b45900bee257b7893cf3be5dba in geode's branch
refs/heads/develop from [~ukohlmeyer]
[ https://git-wip-us.apache.org/repos/asf?p=geode.git;h=a3c0eba ]
GEODE-3386: This now closes #700
> Create Error type for KeyedErrorResponse and ErrorResponse
> --
>
> Key: GEODE-3386
> URL: https://issues.apache.org/jira/browse/GEODE-3386
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Galen O'Sullivan
>
> For logical separation of the new client API, it will be better to have an
> Error that is contained by ErrorResponse, rather than having
> KeyedErrorResponse contain an ErrorResponse.
> In pseudo-protobuf,
> {code}
> PutAllResponse {
> repeated Entry successes = 1,
> repeated KeyedErrorResponse errors = 2,
> }
> KeyedErrorResponse {
> Key,
> ErrorResponse
> }
> ErrorResponse {
> string
> }
> {code}
> instead,
> {code}
> KeyedErrorResponse {
> Key,
> Error,
> }
> {code}
> and
> {code}
> ErrorResponse {
> Error
> }
> Error {
> string
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-3386) Create Error type for KeyedErrorResponse and ErrorResponse
[
https://issues.apache.org/jira/browse/GEODE-3386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16127429#comment-16127429
]
ASF subversion and git services commented on GEODE-3386:
Commit bfbe3e5649158f45f797efcc389f77de88ddaf5a in geode's branch
refs/heads/develop from [~amurmann]
[ https://git-wip-us.apache.org/repos/asf?p=geode.git;h=bfbe3e5 ]
GEODE-3386 - Make KeyedErrorResponse & ErrorResponse siblings
> Create Error type for KeyedErrorResponse and ErrorResponse
> --
>
> Key: GEODE-3386
> URL: https://issues.apache.org/jira/browse/GEODE-3386
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Galen O'Sullivan
>
> For logical separation of the new client API, it will be better to have an
> Error that is contained by ErrorResponse, rather than having
> KeyedErrorResponse contain an ErrorResponse.
> In pseudo-protobuf,
> {code}
> PutAllResponse {
> repeated Entry successes = 1,
> repeated KeyedErrorResponse errors = 2,
> }
> KeyedErrorResponse {
> Key,
> ErrorResponse
> }
> ErrorResponse {
> string
> }
> {code}
> instead,
> {code}
> KeyedErrorResponse {
> Key,
> Error,
> }
> {code}
> and
> {code}
> ErrorResponse {
> Error
> }
> Error {
> string
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-3386) Create Error type for KeyedErrorResponse and ErrorResponse
[
https://issues.apache.org/jira/browse/GEODE-3386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16125956#comment-16125956
]
ASF GitHub Bot commented on GEODE-3386:
---
Github user pivotal-amurmann commented on a diff in the pull request:
https://github.com/apache/geode/pull/700#discussion_r133001811
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
---
@@ -79,9 +81,10 @@
private BasicTypes.KeyedErrorResponse
buildAndLogKeyedError(BasicTypes.Entry entry,
ProtocolErrorCode errorCode, String message, Exception ex) {
logger.error(message, ex);
-BasicTypes.ErrorResponse errorResponse =
BasicTypes.ErrorResponse.newBuilder()
-.setErrorCode(errorCode.codeValue).setMessage(message).build();
-return
BasicTypes.KeyedErrorResponse.newBuilder().setKey(entry.getKey()).setError(errorResponse)
+
+return
BasicTypes.KeyedErrorResponse.newBuilder().setKey(entry.getKey())
+.setError(
+
BasicTypes.Error.newBuilder().setErrorCode(errorCode.codeValue).setMessage(message))
--- End diff --
yeah it did 😔
Tried to reformat it, but it insists on making this bad.
> Create Error type for KeyedErrorResponse and ErrorResponse
> --
>
> Key: GEODE-3386
> URL: https://issues.apache.org/jira/browse/GEODE-3386
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Galen O'Sullivan
>
> For logical separation of the new client API, it will be better to have an
> Error that is contained by ErrorResponse, rather than having
> KeyedErrorResponse contain an ErrorResponse.
> In pseudo-protobuf,
> {code}
> PutAllResponse {
> repeated Entry successes = 1,
> repeated KeyedErrorResponse errors = 2,
> }
> KeyedErrorResponse {
> Key,
> ErrorResponse
> }
> ErrorResponse {
> string
> }
> {code}
> instead,
> {code}
> KeyedErrorResponse {
> Key,
> Error,
> }
> {code}
> and
> {code}
> ErrorResponse {
> Error
> }
> Error {
> string
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-3386) Create Error type for KeyedErrorResponse and ErrorResponse
[
https://issues.apache.org/jira/browse/GEODE-3386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16122533#comment-16122533
]
ASF GitHub Bot commented on GEODE-3386:
---
Github user WireBaron commented on a diff in the pull request:
https://github.com/apache/geode/pull/700#discussion_r132591028
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
---
@@ -41,9 +42,8 @@
String regionName = request.getRegionName();
Region region = cache.getRegion(regionName);
if (region == null) {
- return Failure.of(BasicTypes.ErrorResponse.newBuilder()
-
.setErrorCode(ProtocolErrorCode.REGION_NOT_FOUND.codeValue).setMessage("Region
not found")
- .build());
+ return Failure.of(ProtobufResponseUtilities
+ .makeErrorResponse(ProtocolErrorCode.REGION_NOT_FOUND.codeValue,
"Region not found"));
--- End diff --
Thank you for making this change, this has been bothering me since I added
the error code. Sorry I didn't do this sooner.
> Create Error type for KeyedErrorResponse and ErrorResponse
> --
>
> Key: GEODE-3386
> URL: https://issues.apache.org/jira/browse/GEODE-3386
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Galen O'Sullivan
>
> For logical separation of the new client API, it will be better to have an
> Error that is contained by ErrorResponse, rather than having
> KeyedErrorResponse contain an ErrorResponse.
> In pseudo-protobuf,
> {code}
> PutAllResponse {
> repeated Entry successes = 1,
> repeated KeyedErrorResponse errors = 2,
> }
> KeyedErrorResponse {
> Key,
> ErrorResponse
> }
> ErrorResponse {
> string
> }
> {code}
> instead,
> {code}
> KeyedErrorResponse {
> Key,
> Error,
> }
> {code}
> and
> {code}
> ErrorResponse {
> Error
> }
> Error {
> string
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-3386) Create Error type for KeyedErrorResponse and ErrorResponse
[
https://issues.apache.org/jira/browse/GEODE-3386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16122530#comment-16122530
]
ASF GitHub Bot commented on GEODE-3386:
---
Github user WireBaron commented on a diff in the pull request:
https://github.com/apache/geode/pull/700#discussion_r132591888
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
---
@@ -79,9 +81,10 @@
private BasicTypes.KeyedErrorResponse
buildAndLogKeyedError(BasicTypes.Entry entry,
ProtocolErrorCode errorCode, String message, Exception ex) {
logger.error(message, ex);
-BasicTypes.ErrorResponse errorResponse =
BasicTypes.ErrorResponse.newBuilder()
-.setErrorCode(errorCode.codeValue).setMessage(message).build();
-return
BasicTypes.KeyedErrorResponse.newBuilder().setKey(entry.getKey()).setError(errorResponse)
+
+return
BasicTypes.KeyedErrorResponse.newBuilder().setKey(entry.getKey())
+.setError(
+
BasicTypes.Error.newBuilder().setErrorCode(errorCode.codeValue).setMessage(message))
--- End diff --
Ugh...is this how spotless formatted this?
> Create Error type for KeyedErrorResponse and ErrorResponse
> --
>
> Key: GEODE-3386
> URL: https://issues.apache.org/jira/browse/GEODE-3386
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Galen O'Sullivan
>
> For logical separation of the new client API, it will be better to have an
> Error that is contained by ErrorResponse, rather than having
> KeyedErrorResponse contain an ErrorResponse.
> In pseudo-protobuf,
> {code}
> PutAllResponse {
> repeated Entry successes = 1,
> repeated KeyedErrorResponse errors = 2,
> }
> KeyedErrorResponse {
> Key,
> ErrorResponse
> }
> ErrorResponse {
> string
> }
> {code}
> instead,
> {code}
> KeyedErrorResponse {
> Key,
> Error,
> }
> {code}
> and
> {code}
> ErrorResponse {
> Error
> }
> Error {
> string
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-3386) Create Error type for KeyedErrorResponse and ErrorResponse
[
https://issues.apache.org/jira/browse/GEODE-3386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16122532#comment-16122532
]
ASF GitHub Bot commented on GEODE-3386:
---
Github user WireBaron commented on a diff in the pull request:
https://github.com/apache/geode/pull/700#discussion_r132591701
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
---
@@ -28,6 +28,8 @@
import org.apache.geode.serialization.SerializationService;
import
org.apache.geode.serialization.exception.UnsupportedEncodingTypeException;
import
org.apache.geode.serialization.registry.exception.CodecNotRegisteredForTypeException;
+
+import com.fasterxml.jackson.databind.introspect.TypeResolutionContext;
--- End diff --
I don't see how this new import is being used?
> Create Error type for KeyedErrorResponse and ErrorResponse
> --
>
> Key: GEODE-3386
> URL: https://issues.apache.org/jira/browse/GEODE-3386
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Galen O'Sullivan
>
> For logical separation of the new client API, it will be better to have an
> Error that is contained by ErrorResponse, rather than having
> KeyedErrorResponse contain an ErrorResponse.
> In pseudo-protobuf,
> {code}
> PutAllResponse {
> repeated Entry successes = 1,
> repeated KeyedErrorResponse errors = 2,
> }
> KeyedErrorResponse {
> Key,
> ErrorResponse
> }
> ErrorResponse {
> string
> }
> {code}
> instead,
> {code}
> KeyedErrorResponse {
> Key,
> Error,
> }
> {code}
> and
> {code}
> ErrorResponse {
> Error
> }
> Error {
> string
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-3386) Create Error type for KeyedErrorResponse and ErrorResponse
[
https://issues.apache.org/jira/browse/GEODE-3386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16122531#comment-16122531
]
ASF GitHub Bot commented on GEODE-3386:
---
Github user WireBaron commented on a diff in the pull request:
https://github.com/apache/geode/pull/700#discussion_r132591361
--- Diff:
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
---
@@ -59,13 +59,14 @@
}
return
Success.of(RegionAPI.GetAllResponse.newBuilder().addAllEntries(entries).build());
} catch (UnsupportedEncodingTypeException ex) {
- return Failure.of(BasicTypes.ErrorResponse.newBuilder()
- .setErrorCode(ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue)
- .setMessage("Encoding not supported.").build());
+ int errorCode = ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue;
+ String message = "Encoding not supported.";
+ return
Failure.of(ProtobufResponseUtilities.makeErrorResponse(errorCode, message));
--- End diff --
Having a local variable for the error code and message or inlining them are
both fine approaches, but it's a bit jarring to see both approaches used in two
adjacent code blocks. Please use a consistent approach.
> Create Error type for KeyedErrorResponse and ErrorResponse
> --
>
> Key: GEODE-3386
> URL: https://issues.apache.org/jira/browse/GEODE-3386
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Galen O'Sullivan
>
> For logical separation of the new client API, it will be better to have an
> Error that is contained by ErrorResponse, rather than having
> KeyedErrorResponse contain an ErrorResponse.
> In pseudo-protobuf,
> {code}
> PutAllResponse {
> repeated Entry successes = 1,
> repeated KeyedErrorResponse errors = 2,
> }
> KeyedErrorResponse {
> Key,
> ErrorResponse
> }
> ErrorResponse {
> string
> }
> {code}
> instead,
> {code}
> KeyedErrorResponse {
> Key,
> Error,
> }
> {code}
> and
> {code}
> ErrorResponse {
> Error
> }
> Error {
> string
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (GEODE-3386) Create Error type for KeyedErrorResponse and ErrorResponse
[
https://issues.apache.org/jira/browse/GEODE-3386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16120474#comment-16120474
]
ASF GitHub Bot commented on GEODE-3386:
---
GitHub user pivotal-amurmann opened a pull request:
https://github.com/apache/geode/pull/700
GEODE-3386 - Make KeyedErrorResponse & ErrorResponse siblings
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in
the commit message?
- [x] Has your PR been rebased against the latest commit within the target
branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies
licensed in a way that is compatible for inclusion under [ASF
2.0](http://www.apache.org/legal/resolved.html#category-a)?
N/A
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build
issues and
submit an update to your PR as soon as possible. If you need help, please
send an
email to d...@geode.apache.org.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pivotal-amurmann/geode feature/GEODE-3386
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/700.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #700
commit 81d7a34ef695738c61bf3d288b9ff01f054c30f5
Author: Alexander Murmann
Date: 2017-08-09T18:52:17Z
GEODE-3386 - Make KeyedErrorResponse & ErrorResponse siblings
> Create Error type for KeyedErrorResponse and ErrorResponse
> --
>
> Key: GEODE-3386
> URL: https://issues.apache.org/jira/browse/GEODE-3386
> Project: Geode
> Issue Type: Sub-task
> Components: client/server
>Reporter: Galen O'Sullivan
>
> For logical separation of the new client API, it will be better to have an
> Error that is contained by ErrorResponse, rather than having
> KeyedErrorResponse contain an ErrorResponse.
> In pseudo-protobuf,
> {code}
> PutAllResponse {
> repeated Entry successes = 1,
> repeated KeyedErrorResponse errors = 2,
> }
> KeyedErrorResponse {
> Key,
> ErrorResponse
> }
> ErrorResponse {
> string
> }
> {code}
> instead,
> {code}
> KeyedErrorResponse {
> Key,
> Error,
> }
> {code}
> and
> {code}
> ErrorResponse {
> Error
> }
> Error {
> string
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
