You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/11/22 21:47:03 UTC
[GitHub] [pinot] sajjad-moradi opened a new pull request #7812: Fix and refactor error handling on Controller endpoints
sajjad-moradi opened a new pull request #7812:
URL: https://github.com/apache/pinot/pull/7812
## Description
`FileUploadDownloadClient` uses `ErrorInfo` POJO created in `WebApplicationExceptionMapper` on Controller resources. Currently the json representing the POJO is created using a string. This is not ideal because any refactoring on the POJO doesn't automatically get checked - it's a string and it's not typed-checked at compile time. In fact this happened two times recently and each time the json string got updated as a fix PR (https://github.com/apache/pinot/pull/7428 and https://github.com/apache/pinot/pull/7757).
In this PR, the string manipulation for creating the json is replaced with the actual object and the class representing the object is pulled to a common place, so that `FileUploadDownloadClient` can use the same class for deserialization in a type-checked way.
## Testing Done
Locally added a small storage quota to generate a quota error in `OfflineClusterIntegrationTest`.
Error message before the fix:
```
Got error status code: 403 (Forbidden) with reason: "Failed to get reason" while sending request: http://localhost:18998/v2/segments?tableName=mytable to controller: smoradi-mn1.linkedin.biz, version: Unknown
```
Error message after the fix:
```
Got error status code: 403 (Forbidden) with reason: "Quota check failed for segment: mytable_16282_16312_10 % of table: mytable_OFFLINE, reason: Storage quota exceeded for Table mytable_OFFLINE. New estimated size: 1.57M > total allowed storage size: 1K, where new estimated size = existing estimated uncompressed size of all replicas: 0B - existing segment sizes of all replicas: 0B + (incoming uncompressed segment size: 1.57M * number replicas: 1), total allowed storage size = configured quota: 1K * number replicas: 1" while sending request: http://localhost:18998/v2/segments?tableName=mytable to controller: smoradi-mn1.linkedin.biz, version: Unknown
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #7812: Fix and refactor error handling on Controller endpoints
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7812:
URL: https://github.com/apache/pinot/pull/7812#discussion_r754721903
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -543,9 +543,10 @@ private static String getErrorMessage(HttpUriRequest request, CloseableHttpRespo
StatusLine statusLine = response.getStatusLine();
String reason;
try {
- reason = JsonUtils.stringToJsonNode(EntityUtils.toString(response.getEntity())).get("_error").asText();
+ String entityStr = EntityUtils.toString(response.getEntity());
+ reason = JsonUtils.stringToObject(entityStr, SimpleHttpErrorInfo.class).getError();
Review comment:
I understand that we didn't change the properties in this PR. The concern I have here is that client code and server code may not run the same version. Since the error message can come from either `error` or `_error`, should we consider the backward compatibility here, like trying to fetch both fields?
One scenario I can think of is that if the response still shows `_error` in the payload, when the `stringToObject()` method in Line 547 gets invoked, it will still fail because no `_error` is specified as a JsonProperty. In this case, we still cannot parse the payload correctly.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli merged pull request #7812: Fix and refactor error handling on Controller endpoints
Posted by GitBox <gi...@apache.org>.
jackjlli merged pull request #7812:
URL: https://github.com/apache/pinot/pull/7812
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #7812: Fix and refactor error handling on Controller endpoints
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7812:
URL: https://github.com/apache/pinot/pull/7812#discussion_r754666677
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -543,9 +543,10 @@ private static String getErrorMessage(HttpUriRequest request, CloseableHttpRespo
StatusLine statusLine = response.getStatusLine();
String reason;
try {
- reason = JsonUtils.stringToJsonNode(EntityUtils.toString(response.getEntity())).get("_error").asText();
+ String entityStr = EntityUtils.toString(response.getEntity());
+ reason = JsonUtils.stringToObject(entityStr, SimpleHttpErrorInfo.class).getError();
Review comment:
Have we considered backward compatibility here since the server and client may not use the same version at all times (e.g. during deployment)?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/WebApplicationExceptionMapper.java
##########
@@ -25,6 +25,7 @@
import javax.ws.rs.core.Response;
import javax.ws.rs.ext.ExceptionMapper;
import javax.ws.rs.ext.Provider;
+import org.apache.pinot.common.utils.SimpleHttpErrorInfo;
Review comment:
You might need to remove the imports of JsonCreator and JsonProperty in this class.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] sajjad-moradi commented on a change in pull request #7812: Fix and refactor error handling on Controller endpoints
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #7812:
URL: https://github.com/apache/pinot/pull/7812#discussion_r754717741
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -543,9 +543,10 @@ private static String getErrorMessage(HttpUriRequest request, CloseableHttpRespo
StatusLine statusLine = response.getStatusLine();
String reason;
try {
- reason = JsonUtils.stringToJsonNode(EntityUtils.toString(response.getEntity())).get("_error").asText();
+ String entityStr = EntityUtils.toString(response.getEntity());
+ reason = JsonUtils.stringToObject(entityStr, SimpleHttpErrorInfo.class).getError();
Review comment:
The incompatibility issue you're referring to comes into picture when/if the object definition is changed. This PR only refactors the way the POJO is deserialized. It does not change the properties of object itself.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] sajjad-moradi commented on a change in pull request #7812: Fix and refactor error handling on Controller endpoints
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #7812:
URL: https://github.com/apache/pinot/pull/7812#discussion_r754741458
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -543,9 +543,10 @@ private static String getErrorMessage(HttpUriRequest request, CloseableHttpRespo
StatusLine statusLine = response.getStatusLine();
String reason;
try {
- reason = JsonUtils.stringToJsonNode(EntityUtils.toString(response.getEntity())).get("_error").asText();
+ String entityStr = EntityUtils.toString(response.getEntity());
+ reason = JsonUtils.stringToObject(entityStr, SimpleHttpErrorInfo.class).getError();
Review comment:
This is a valid concern and we'll have incompatibility between different versions for different component. However, as I mentioned before, this PR is just a refactoring and the original backward incompatible changes in the data definition shouldn't have been allowed in the first place. Since this is not in the success path and we'll know about the error by catching the exception, I believe it makes the code simpler and more maintainable if we don't have a special code handling "error", "_error", and any future names.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org