Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parsing error messages returned to FileUploadDownloadClient #7428

Conversation

sajjad-moradi
Copy link
Contributor

@sajjad-moradi sajjad-moradi commented Sep 14, 2021

Description

FileUploadDownloadClient uses a wrong field name to extract the error message from Entity of HttpResponse.
Sample response entity:

{
  "_code": 403,
  "_error": "Quota check failed for segment: mytable_16405_16435_2 % of table: mytable_OFFLINE, reason: Storage quota exceeded for Table mytable_OFFLINE. New estimated size: 1.43M > 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.43M * number replicas: 1), total allowed storage size = configured quota: 1K * number replicas: 1"
}

This PR uses the correct field name.

Testing Done

Modified OfflineClusterIntegrationTest to generate a quota error.
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

Error message after the fix:

Got error status code: 403 (Forbidden) with reason: "Quota check failed for segment: mytable_16374_16404_1 % of table: mytable_OFFLINE, reason: Storage quota exceeded for Table mytable_OFFLINE. New estimated size: 1.33M > 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.33M * number replicas: 1), total allowed storage size = configured quota: 1K * number replicas: 1" while sending request: http://localhost:18998/v2/segments?tableName=mytable

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2021

Codecov Report

Merging #7428 (3467be4) into master (0ee33e8) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7428      +/-   ##
============================================
- Coverage     69.83%   69.76%   -0.07%     
+ Complexity     3258     3251       -7     
============================================
  Files          1123     1123              
  Lines         53133    53133              
  Branches       8008     8008              
============================================
- Hits          37106    37069      -37     
- Misses        13397    13433      +36     
- Partials       2630     2631       +1     
Flag Coverage Δ
unittests1 69.76% <0.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...e/pinot/common/utils/FileUploadDownloadClient.java 19.81% <0.00%> (ø)
...core/startree/operator/StarTreeFilterOperator.java 68.08% <0.00%> (-15.61%) ⬇️
...nt/local/startree/v2/store/StarTreeDataSource.java 40.00% <0.00%> (-13.34%) ⬇️
...ore/query/scheduler/resources/ResourceManager.java 85.71% <0.00%> (-10.72%) ⬇️
...ot/segment/local/startree/OffHeapStarTreeNode.java 72.22% <0.00%> (-5.56%) ⬇️
.../startree/v2/builder/OffHeapSingleTreeBuilder.java 87.42% <0.00%> (-4.20%) ⬇️
...e/pinot/core/transport/InstanceRequestHandler.java 56.94% <0.00%> (-4.17%) ⬇️
.../pinot/core/query/scheduler/PriorityScheduler.java 78.08% <0.00%> (-2.74%) ⬇️
.../core/operator/combine/GroupByCombineOperator.java 75.40% <0.00%> (-1.64%) ⬇️
...cal/startree/v2/builder/BaseSingleTreeBuilder.java 88.28% <0.00%> (-0.91%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ee33e8...3467be4. Read the comment docs.

@@ -509,7 +509,7 @@ private static String getErrorMessage(HttpUriRequest request, CloseableHttpRespo
StatusLine statusLine = response.getStatusLine();
String reason;
try {
reason = JsonUtils.stringToJsonNode(EntityUtils.toString(response.getEntity())).get("error").asText();
reason = JsonUtils.stringToJsonNode(EntityUtils.toString(response.getEntity())).get("_error").asText();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you put a sample entity to the description of this PR to show what is included in the entity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -509,7 +509,7 @@ private static String getErrorMessage(HttpUriRequest request, CloseableHttpRespo
StatusLine statusLine = response.getStatusLine();
String reason;
try {
reason = JsonUtils.stringToJsonNode(EntityUtils.toString(response.getEntity())).get("error").asText();
reason = JsonUtils.stringToJsonNode(EntityUtils.toString(response.getEntity())).get("_error").asText();
} catch (Exception e) {
reason = "Failed to get reason";
Copy link
Contributor

@jackjlli jackjlli Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also print out some useful info (like the raw output) if any exception is throw when parsing the entity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole purpose of this method, getErrorMessage is to have a nicer log message. It defeats its purpose if we log/print out something here. With the bug fix, it should work as intended.

Copy link
Contributor

@jackjlli jackjlli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for working on this!

@jackjlli
Copy link
Contributor

The integration test keeps failing on this test:

[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    MergeRollupMinionClusterIntegrationTest.testMultiLevelConcat:543 expected [10] but found [7]
[INFO] 
Error:  Tests run: 99, Failures: 1, Errors: 0, Skipped: 0

Let's wait for the following PR to be merged before merging this PR:
#7432

@snleee
Copy link
Contributor

snleee commented Sep 15, 2021

@jackjlli @sajjad-moradi Maybe we should rebase based on the master branch and re-trigger the test.

@sajjad-moradi sajjad-moradi force-pushed the bug/fix.error.message.issue.in.FileUploadDownloadClient branch from 2bb2fd0 to 13e56ee Compare September 15, 2021 21:05
@sajjad-moradi sajjad-moradi force-pushed the bug/fix.error.message.issue.in.FileUploadDownloadClient branch from 13e56ee to 3467be4 Compare September 16, 2021 22:00
@jackjlli jackjlli merged commit 2dbc982 into apache:master Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants