You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/03/28 13:16:44 UTC
[GitHub] [iceberg] singhpk234 opened a new pull request #4422: Core: use primitive type in error code in ErrorResponse
singhpk234 opened a new pull request #4422:
URL: https://github.com/apache/iceberg/pull/4422
This change attempts to use primitive type in code property of Error Response, As per my understanding since we use our own parser i.e ErrorResponseParser we should be fine.
Don't have strong reason to support why this is required, would say something that caught my eye and in general we should use primitive type over boxed.
---
cc @kbendick
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a change in pull request #4422: Core: use primitive type in error code in ErrorResponse
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4422:
URL: https://github.com/apache/iceberg/pull/4422#discussion_r837047534
##########
File path: core/src/test/java/org/apache/iceberg/rest/responses/TestErrorResponseParser.java
##########
@@ -40,14 +41,25 @@ public void testErrorResponseToJson() {
public void testErrorResponseFromJson() {
String message = "The given namespace does not exist";
String type = "NoSuchNamespaceException";
- Integer code = 404;
+ int code = 404;
String errorModelJson = String.format("{\"message\":\"%s\",\"type\":\"%s\",\"code\":%d}", message, type, code);
String json = "{\"error\":" + errorModelJson + "}";
ErrorResponse expected = ErrorResponse.builder().withMessage(message).withType(type).responseCode(code).build();
assertEquals(expected, ErrorResponseParser.fromJson(json));
}
+ @Test
+ public void testErrorResponseFromJsonWithoutCode() {
+ String message = "The given namespace does not exist";
+ String type = "NoSuchNamespaceException";
+ String errorModelJson = String.format("{\"message\":\"%s\",\"type\":\"%s\"}", message, type);
+ String json = "{\"error\":" + errorModelJson + "}";
+
+ AssertHelpers.assertThrows("ErrorResponse should contain code", IllegalArgumentException.class,
+ "Cannot parse missing int code", () -> ErrorResponseParser.fromJson(json));
+ }
Review comment:
> I'm not sure if adding this additional constraint provides that much benefit.
+1, I also expected that code should be present anyways, absence of it during build() time we check code is not null for creating Response obj, and throw [IllegalArgs exception](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java#L132).
> I think this optimization is possibly a bit premature, but I'll let others weigh in as well first.
Agree with you, I don't have a strong opinion of must having this change either. Happy to close this as well :) !!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a change in pull request #4422: Core: use primitive type in error code in ErrorResponse
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4422:
URL: https://github.com/apache/iceberg/pull/4422#discussion_r837047534
##########
File path: core/src/test/java/org/apache/iceberg/rest/responses/TestErrorResponseParser.java
##########
@@ -40,14 +41,25 @@ public void testErrorResponseToJson() {
public void testErrorResponseFromJson() {
String message = "The given namespace does not exist";
String type = "NoSuchNamespaceException";
- Integer code = 404;
+ int code = 404;
String errorModelJson = String.format("{\"message\":\"%s\",\"type\":\"%s\",\"code\":%d}", message, type, code);
String json = "{\"error\":" + errorModelJson + "}";
ErrorResponse expected = ErrorResponse.builder().withMessage(message).withType(type).responseCode(code).build();
assertEquals(expected, ErrorResponseParser.fromJson(json));
}
+ @Test
+ public void testErrorResponseFromJsonWithoutCode() {
+ String message = "The given namespace does not exist";
+ String type = "NoSuchNamespaceException";
+ String errorModelJson = String.format("{\"message\":\"%s\",\"type\":\"%s\"}", message, type);
+ String json = "{\"error\":" + errorModelJson + "}";
+
+ AssertHelpers.assertThrows("ErrorResponse should contain code", IllegalArgumentException.class,
+ "Cannot parse missing int code", () -> ErrorResponseParser.fromJson(json));
+ }
Review comment:
> I'm not sure if adding this additional constraint provides that much benefit.
+1, I also expected that code should be present anyways, absence of it during build() time we check code is not null for creating Response obj, and throw [IllegalArgs exception](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java#L132).
The thing that caught my eye was this :
https://github.com/apache/iceberg/blob/513169d95b379ca723b6217681158b4ea1e09d85/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java#L35
here code is already `int` and in the getter we were boxing it to `Integer`
https://github.com/apache/iceberg/blob/513169d95b379ca723b6217681158b4ea1e09d85/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java#L58-L60
> I think this optimization is possibly a bit premature, but I'll let others weigh in as well first.
Agree with you, I don't have a strong opinion of must having this change either. Happy to close this as well :) !!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a change in pull request #4422: Core: use primitive type in error code in ErrorResponse
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4422:
URL: https://github.com/apache/iceberg/pull/4422#discussion_r839191558
##########
File path: core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponseParser.java
##########
@@ -91,7 +91,7 @@ public static ErrorResponse fromJson(JsonNode jsonNode) {
JsonNode error = jsonNode.get(ERROR);
String message = JsonUtil.getStringOrNull(MESSAGE, error);
String type = JsonUtil.getStringOrNull(TYPE, error);
- Integer code = JsonUtil.getIntOrNull(CODE, error);
+ int code = JsonUtil.getInt(CODE, error);
Review comment:
Not at all a problem :) !!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4422: Core: use primitive type in error code in ErrorResponse
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4422:
URL: https://github.com/apache/iceberg/pull/4422#discussion_r836787865
##########
File path: core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponseParser.java
##########
@@ -91,7 +91,7 @@ public static ErrorResponse fromJson(JsonNode jsonNode) {
JsonNode error = jsonNode.get(ERROR);
String message = JsonUtil.getStringOrNull(MESSAGE, error);
String type = JsonUtil.getStringOrNull(TYPE, error);
- Integer code = JsonUtil.getIntOrNull(CODE, error);
+ int code = JsonUtil.getInt(CODE, error);
Review comment:
Question: if `CODE` isn't present, what exception will this throw?
##########
File path: core/src/test/java/org/apache/iceberg/rest/responses/TestErrorResponseParser.java
##########
@@ -40,14 +41,25 @@ public void testErrorResponseToJson() {
public void testErrorResponseFromJson() {
String message = "The given namespace does not exist";
String type = "NoSuchNamespaceException";
- Integer code = 404;
+ int code = 404;
String errorModelJson = String.format("{\"message\":\"%s\",\"type\":\"%s\",\"code\":%d}", message, type, code);
String json = "{\"error\":" + errorModelJson + "}";
ErrorResponse expected = ErrorResponse.builder().withMessage(message).withType(type).responseCode(code).build();
assertEquals(expected, ErrorResponseParser.fromJson(json));
}
+ @Test
+ public void testErrorResponseFromJsonWithoutCode() {
+ String message = "The given namespace does not exist";
+ String type = "NoSuchNamespaceException";
+ String errorModelJson = String.format("{\"message\":\"%s\",\"type\":\"%s\"}", message, type);
+ String json = "{\"error\":" + errorModelJson + "}";
+
+ AssertHelpers.assertThrows("ErrorResponse should contain code", IllegalArgumentException.class,
+ "Cannot parse missing int code", () -> ErrorResponseParser.fromJson(json));
+ }
Review comment:
My concern here is that the error code can still be parsed from other places (like the response itself). I agree it _should_ be there, but I'm not sure if adding this additional constraint provides that much benefit.
The number of boxed `Integer`s is still likely to be relatively small, as most operations will still go through normal `FileIO`.
I think this optimization is possibly a bit premature, but I'll let others weigh in as well first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4422: Core: use primitive type in error code in ErrorResponse
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4422:
URL: https://github.com/apache/iceberg/pull/4422#discussion_r838854721
##########
File path: core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponseParser.java
##########
@@ -91,7 +91,7 @@ public static ErrorResponse fromJson(JsonNode jsonNode) {
JsonNode error = jsonNode.get(ERROR);
String message = JsonUtil.getStringOrNull(MESSAGE, error);
String type = JsonUtil.getStringOrNull(TYPE, error);
- Integer code = JsonUtil.getIntOrNull(CODE, error);
+ int code = JsonUtil.getInt(CODE, error);
Review comment:
Oh my bad. The unit test completely shows that. I've been somewhat sick the past few days so forgive me 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a change in pull request #4422: Core: use primitive type in error code in ErrorResponse
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4422:
URL: https://github.com/apache/iceberg/pull/4422#discussion_r837043675
##########
File path: core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponseParser.java
##########
@@ -91,7 +91,7 @@ public static ErrorResponse fromJson(JsonNode jsonNode) {
JsonNode error = jsonNode.get(ERROR);
String message = JsonUtil.getStringOrNull(MESSAGE, error);
String type = JsonUtil.getStringOrNull(TYPE, error);
- Integer code = JsonUtil.getIntOrNull(CODE, error);
+ int code = JsonUtil.getInt(CODE, error);
Review comment:
It would throw an IllegalArgumentException, the UT added with this PR shows the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org