Updated the response for reload API to be pretty printed #11608

ArnavBalyan · 2023-09-17T18:13:20Z

Reload API returns an encoded JSON which is hard to understand.
Updated the response to remove the additional string.
Added a Pretty printed string which will print the json in an understandable format.
performance fix for Reload API has a difficult response #11588

Jackie-Jiang

cc @jadami10 to also provide some feedback

Jackie-Jiang · 2023-09-17T23:54:59Z

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

@@ -782,7 +782,7 @@ public SuccessResponse reloadAllSegments(
        LOGGER.error("Failed to add reload all segments job meta into zookeeper for table: {}", tableNameWithType, e);
      }
    }
-    return new SuccessResponse("Segment reload details: " + JsonUtils.objectToString(perTableMsgData));
+    return new SuccessResponse(JsonUtils.objectToPrettyString(perTableMsgData));


It doesn't need to be a json, maybe simply

Suggested change

return new SuccessResponse(JsonUtils.objectToPrettyString(perTableMsgData));

return new SuccessResponse("Segment reload details: " + perTableMsgData);

it doesn't need to be, but what value is Segment reload details: to the result of an endpoint that is clearly already the reload details. The impetus for this change was to make it easier to parse the result in java/scala so you don't have to go from request -> strip the prefix -> decode the json string -> decode the json. You should be able to go from request -> json in 1 hop.

For now, I've just kept the json string in the success response which makes it easier to parse/understand. Please let me know what you think Thanks!

codecov-commenter · 2023-09-18T05:46:47Z

Codecov Report

Merging #11608 (4057e25) into master (0cf091a) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master   #11608      +/-   ##
============================================
- Coverage     66.36%   66.34%   -0.03%     
  Complexity      207      207              
============================================
  Files          2350     2350              
  Lines        127248   127248              
  Branches      19593    19593              
============================================
- Hits          84449    84420      -29     
- Misses        36912    36935      +23     
- Partials       5887     5893       +6

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (ø)`
integration	`<0.01% <0.00%> (ø)`
integration1	`<0.01% <0.00%> (ø)`
integration2	`0.00% <0.00%> (ø)`
java-11	`49.98% <0.00%> (-16.33%)`	⬇️
java-17	`66.20% <100.00%> (+<0.01%)`	⬆️
java-20	`66.20% <100.00%> (-0.03%)`	⬇️
temurin	`66.34% <100.00%> (-0.03%)`	⬇️
unittests	`66.34% <100.00%> (-0.03%)`	⬇️
unittests1	`67.13% <ø> (+<0.01%)`	⬆️
unittests2	`17.80% <100.00%> (-0.03%)`	⬇️

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

Files	Coverage Δ
...ler/api/resources/PinotSegmentRestletResource.java	`41.13% <100.00%> (ø)`

... and 19 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jadami10

you also need to update

pinot/pinot-controller/src/main/resources/app/pages/TenantDetails.tsx

Line 340 in b1b070f

    
           const statusResponseObj = JSON.parse(result.status.replace("Segment reload details: ", ""))

since that's parsing this result.

You should run this through the quickstart to make sure it's still working

jadami10 · 2023-09-18T13:23:56Z

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

@@ -782,7 +782,7 @@ public SuccessResponse reloadAllSegments(
        LOGGER.error("Failed to add reload all segments job meta into zookeeper for table: {}", tableNameWithType, e);
      }
    }
-    return new SuccessResponse("Segment reload details: " + JsonUtils.objectToString(perTableMsgData));
+    return new SuccessResponse(JsonUtils.objectToPrettyString(perTableMsgData));


it doesn't need to be, but what value is Segment reload details: to the result of an endpoint that is clearly already the reload details. The impetus for this change was to make it easier to parse the result in java/scala so you don't have to go from request -> strip the prefix -> decode the json string -> decode the json. You should be able to go from request -> json in 1 hop.

ArnavBalyan · 2023-09-18T13:39:38Z

you also need to update

pinot/pinot-controller/src/main/resources/app/pages/TenantDetails.tsx

Line 340 in b1b070f

const statusResponseObj = JSON.parse(result.status.replace("Segment reload details: ", ""))

since that's parsing this result.
You should run this through the quickstart to make sure it's still working

Got it thanks let me do this

jadami10 · 2023-09-20T04:02:43Z

looks like there's still some tests failing

Jackie-Jiang · 2023-10-04T23:09:04Z

...on-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java

    String tableNameWithType = TableNameBuilder.forType(tableType).tableNameWithType(tableName);
    JsonNode tableLevelDetails =
-        JsonUtils.stringToJsonNode(StringEscapeUtils.unescapeJava(response.split(": ")[1])).get(tableNameWithType);
+        JsonUtils.stringToJsonNode(response.substring(


This is very hard to understand. I think we might need to parse twice? Something like:

JsonNode responseJson = JsonUtils.stringToJsonNode(response); JsonNode tableLevelDetails = JsonUtils.stringToJsonNode(responseJson.get("status").asText()).get(tableNameWithType);

Hi @Jackie-Jiang the the value corresponding to the "status" is another json string. So parsing it into json fails because the value is wrapped in "". I just did the substring logic to get the wrapped string. Thanks!

The json string is encoded as the value field of the top level json, so parsing the json twice should be able to extract it. See the code example above

Got it thanks, I did this, it seems the 6 tests fail. The result is wrapped in "", so the JsonUtils.stringToJsonNode fails parsing it, to fix the test we would have to extract the value and then parse it. What do you think. Thanks!

The problem is the unnecessary unescape. Applied a commit and let's see if it fixes the problem

Oh got it thank you so much!

ArnavBalyan changed the title ~~Fix for #11588. Updated the response for reload API to be pretty printed~~ Updated the response for reload API to be pretty printed Sep 17, 2023

Jackie-Jiang reviewed Sep 17, 2023

View reviewed changes

Jackie-Jiang added the rest-api label Sep 17, 2023

jadami10 reviewed Sep 18, 2023

View reviewed changes

ArnavBalyan force-pushed the arnavb/11588 branch from fe3f7f8 to cda8d11 Compare September 19, 2023 12:51

ArnavBalyan added 5 commits October 3, 2023 19:30

updated the response to prettyString

f9a9f6f

update

0c081b8

update response

e776297

fix test

3674fb1

checking test

f98781f

ArnavBalyan force-pushed the arnavb/11588 branch from c46f1b4 to f98781f Compare October 3, 2023 14:00

ArnavBalyan added 2 commits October 4, 2023 01:19

update test

ddd55e0

lint fix

3b6ca25

Jackie-Jiang reviewed Oct 4, 2023

View reviewed changes

update

c803385

ArnavBalyan force-pushed the arnavb/11588 branch from 6245222 to c803385 Compare October 16, 2023 19:53

Jackie-Jiang added 2 commits October 18, 2023 15:52

Update BaseClusterIntegrationTestSet.java

396e704

Merge branch 'master' into arnavb/11588

4057e25

Jackie-Jiang approved these changes Oct 19, 2023

View reviewed changes

Jackie-Jiang merged commit 0d08316 into apache:master Oct 19, 2023
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated the response for reload API to be pretty printed #11608

Updated the response for reload API to be pretty printed #11608

ArnavBalyan commented Sep 17, 2023 •

edited

Jackie-Jiang left a comment

Jackie-Jiang Sep 17, 2023

jadami10 Sep 18, 2023

ArnavBalyan Sep 19, 2023

codecov-commenter commented Sep 18, 2023 •

edited

jadami10 left a comment

jadami10 Sep 18, 2023

ArnavBalyan commented Sep 18, 2023

jadami10 commented Sep 20, 2023

Jackie-Jiang Oct 4, 2023

ArnavBalyan Oct 5, 2023

Jackie-Jiang Oct 6, 2023

ArnavBalyan Oct 17, 2023

Jackie-Jiang Oct 18, 2023

ArnavBalyan Oct 19, 2023

	return new SuccessResponse(JsonUtils.objectToPrettyString(perTableMsgData));
	return new SuccessResponse("Segment reload details: " + perTableMsgData);

Updated the response for reload API to be pretty printed #11608

Updated the response for reload API to be pretty printed #11608

Conversation

ArnavBalyan commented Sep 17, 2023 • edited

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 18, 2023 • edited

Codecov Report

jadami10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArnavBalyan commented Sep 18, 2023

jadami10 commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArnavBalyan commented Sep 17, 2023 •

edited

codecov-commenter commented Sep 18, 2023 •

edited