Move Other HttpUtils to use HttpClient by walterddr · Pull Request #8390 · apache/pinot

walterddr · 2022-03-23T03:21:42Z

follow up on #8329 .

Description

We still have several http utils in the codebase that we should migrate to use the new HttpClient. This PR moves:

ControllerTest
pinot-flink-connector HttpClient

TODO

After this PR there are still some utilities that directly uses java.net.* or apache.http.* without an HttpUtils class. which we can migrate later.

codecov-commenter · 2022-03-23T19:11:47Z

Codecov Report

Merging #8390 (46f2ec2) into master (6d2f749) will decrease coverage by 43.30%.
The diff coverage is 51.33%.

❗ Current head 46f2ec2 differs from pull request most recent head 52f1ae0. Consider uploading reports for the commit 52f1ae0 to get more accurate results

@@              Coverage Diff              @@
##             master    #8390       +/-   ##
=============================================
- Coverage     70.79%   27.49%   -43.31%     
=============================================
  Files          1654     1643       -11     
  Lines         86464    86387       -77     
  Branches      13037    13037               
=============================================
- Hits          61216    23749    -37467     
- Misses        21013    60404    +39391     
+ Partials       4235     2234     -2001

Flag	Coverage Δ
integration1	`?`
integration2	`27.49% <51.33%> (+0.28%)`	⬆️
unittests1	`?`
unittests2	`?`

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

Impacted Files	Coverage Δ
...he/pinot/common/function/scalar/JsonFunctions.java	`25.75% <0.00%> (-62.13%)`	⬇️
...a/org/apache/pinot/common/metrics/ServerGauge.java	`95.65% <ø> (-0.19%)`	⬇️
...apache/pinot/controller/BaseControllerStarter.java	`75.07% <ø> (-7.26%)`	⬇️
...va/org/apache/pinot/controller/ControllerConf.java	`50.20% <ø> (-8.51%)`	⬇️
.../controller/helix/ControllerRequestURLBuilder.java	`18.18% <0.00%> (-65.79%)`	⬇️
.../transform/function/DateTimeTransformFunction.java	`0.00% <0.00%> (ø)`
...ansformer/datetime/DateTimeTransformerFactory.java	`58.33% <0.00%> (-41.67%)`	⬇️
...ache/pinot/core/query/reduce/GapfillProcessor.java	`0.00% <0.00%> (-93.68%)`	⬇️
...gment/local/function/InbuiltFunctionEvaluator.java	`0.00% <0.00%> (-88.71%)`	⬇️
...local/recordtransformer/ExpressionTransformer.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 1222 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 6d2f749...52f1ae0. Read the comment docs.

- sleep for 1s for controller to restart in restart controller tests - change check FileNotFoundException to check status code 404 - fix reloadSegment/Table

Jackie-Jiang

Nice! Much cleaner

pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClient.java

pinot-connectors/pinot-flink-connector/pom.xml

pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java

Jackie-Jiang · 2022-03-28T17:02:47Z

...c/test/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManagerStatelessTest.java

    stopController();
    startController(properties);
+    // wait for controller to start correctly.
+    Thread.sleep(1000);


Do you run into issues without this wait? We should avoid using this kind of wait to get instance ready as this can be flaky

yeah. for some unknown reason this test will fail if I dont wait. i can do a "TestUtils.waitUntilCondition" instead.

Let's check what is causing the flakiness, and use TestUtils.waitUntilCondition to guard that condition

i think we just need to make sure the HTTPServer is up . see my change

Why is the HTTP server still not ready when the start controller already returns? IMO it should be ready during the start

this I have no idea. the error simply said

Mar 28, 2022 6:31:51 PM org.glassfish.grizzly.http.server.NetworkListener shutdownNow INFO: Stopped listener bound to [0.0.0.0:18998] Mar 28, 2022 6:31:52 PM org.glassfish.grizzly.http.server.NetworkListener start INFO: Started listener bound to [0.0.0.0:18998] Mar 28, 2022 6:31:52 PM org.glassfish.grizzly.http.server.HttpServer start INFO: [HttpServer-1] Started. org.apache.http.NoHttpResponseException: localhost:18998 failed to respond at org.apache.http.impl.conn.DefaultHttpResponseParser.parseHead(DefaultHttpResponseParser.java:141) at org.apache.http.impl.conn.DefaultHttpResponseParser.parseHead(DefaultHttpResponseParser.java:56) at org.apache.http.impl.io.AbstractMessageParser.parse(AbstractMessageParser.java:259) at org.apache.http.impl.DefaultBHttpClientConnection.receiveResponseHeader(DefaultBHttpClientConnection.java:163) at org.apache.http.impl.conn.CPoolProxy.receiveResponseHeader(CPoolProxy.java:157) at org.apache.http.protocol.HttpRequestExecutor.doReceiveResponse(HttpRequestExecutor.java:273) at org.apache.http.protocol.HttpRequestExecutor.execute(HttpRequestExecutor.java:125) at org.apache.http.impl.execchain.MainClientExec.execute(MainClientExec.java:272) at org.apache.http.impl.execchain.ProtocolExec.execute(ProtocolExec.java:186) at org.apache.http.impl.execchain.RetryExec.execute(RetryExec.java:89) at org.apache.http.impl.execchain.RedirectExec.execute(RedirectExec.java:110) at org.apache.http.impl.client.InternalHttpClient.doExecute(InternalHttpClient.java:185) ... at org.apache.pinot.common.utils.http.HttpClient.sendJsonPutRequest(HttpClient.java:257) at org.apache.pinot.controller.helix.ControllerRequestClient.updateTableConfig(ControllerRequestClient.java:100) at org.apache.pinot.controller.helix.ControllerTest.updateTableConfig(ControllerTest.java:540) at org.apache.pinot.controller.helix.core.minion.PinotTaskManagerStatelessTest.testPinotTaskManagerSchedulerWithRestart(PinotTaskManagerStatelessTest.java:180)

we should separately investigate the issue IMO

@walterddr can you create an issue to track the above ? I think we can merge this

done see #8431

walterddr force-pushed the move_rest_of_http_utils branch 2 times, most recently from f406acd to 5d2f2ef Compare March 23, 2022 15:46

walterddr marked this pull request as ready for review March 23, 2022 15:49

walterddr marked this pull request as draft March 23, 2022 15:49

walterddr marked this pull request as ready for review March 23, 2022 17:32

walterddr marked this pull request as draft March 23, 2022 17:32

walterddr force-pushed the move_rest_of_http_utils branch from 5d2f2ef to c48b7b5 Compare March 23, 2022 17:33

walterddr force-pushed the move_rest_of_http_utils branch from f9bd3fc to e4bdd9c Compare March 23, 2022 23:34

flink http utils

c4f1594

walterddr force-pushed the move_rest_of_http_utils branch from e4bdd9c to db16a75 Compare March 24, 2022 16:59

introduce a static HttpClient for testing purpose

68f7f22

walterddr force-pushed the move_rest_of_http_utils branch from db16a75 to 68f7f22 Compare March 24, 2022 17:17

Rong Rong added 2 commits March 24, 2022 21:02

adjust tests

8ac708e

- sleep for 1s for controller to restart in restart controller tests - change check FileNotFoundException to check status code 404 - fix reloadSegment/Table

fix test again

8813316

walterddr force-pushed the move_rest_of_http_utils branch from 905036f to 8813316 Compare March 25, 2022 17:01

fix controller test http static reference

fa65207

walterddr force-pushed the move_rest_of_http_utils branch from 3976a3b to fa65207 Compare March 26, 2022 16:16

walterddr changed the title ~~Migrate HttpUtils to use HttpClient~~ Move Other HttpUtils to use HttpClient Mar 26, 2022

fix API test

52f1ae0

walterddr marked this pull request as ready for review March 27, 2022 03:45

Jackie-Jiang approved these changes Mar 28, 2022

View reviewed changes

address diff comments

f67bf4a

siddharthteotia merged commit 189fd7a into apache:master Mar 29, 2022

This was referenced Mar 29, 2022

Potential controller start/stop race condition #8431

Open

Java client for Controller/Broker/Server REST APIs #8246

Open

walterddr deleted the move_rest_of_http_utils branch December 6, 2023 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Other HttpUtils to use HttpClient#8390

Move Other HttpUtils to use HttpClient#8390
siddharthteotia merged 7 commits intoapache:masterfrom
walterddr:move_rest_of_http_utils

walterddr commented Mar 23, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Mar 23, 2022 •

edited

Loading

Uh oh!

Jackie-Jiang left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang Mar 28, 2022

Uh oh!

walterddr Mar 28, 2022

Uh oh!

Jackie-Jiang Mar 28, 2022

Uh oh!

walterddr Mar 28, 2022

Uh oh!

Jackie-Jiang Mar 29, 2022

Uh oh!

walterddr Mar 29, 2022 •

edited

Loading

Uh oh!

siddharthteotia Mar 29, 2022

Uh oh!

walterddr Mar 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

walterddr commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO

Uh oh!

codecov-commenter commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

walterddr Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

walterddr Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

walterddr Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthteotia Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

walterddr Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

walterddr commented Mar 23, 2022 •

edited

Loading

codecov-commenter commented Mar 23, 2022 •

edited

Loading

walterddr Mar 29, 2022 •

edited

Loading