Skip to content

Use MultiThreadedHttpConnectionManager in SegmentStatusChecker#8568

Merged
siddharthteotia merged 1 commit intoapache:masterfrom
vvivekiyer:http_connection_manager
Apr 20, 2022
Merged

Use MultiThreadedHttpConnectionManager in SegmentStatusChecker#8568
siddharthteotia merged 1 commit intoapache:masterfrom
vvivekiyer:http_connection_manager

Conversation

@vvivekiyer
Copy link
Contributor

@vvivekiyer vvivekiyer commented Apr 20, 2022

Currently, SegmentStatusChecker initializes an object of type
SimpleHttpConnectionManager. Further down the stack, we use this object to
issue parallel HTTP GET requests to multiple servers containing segments for
the table (in MultiGetRequest). However, SimpleHttpConnectionManager is not
thread safe. So we should be using an instance of MultithreadedHttpConnectionManager.

Added a TODO to improve MultiGetRequest.java to only work on HttpConnectionManager
object that is an instanceOf MultithreadedHttpConnectionManager.

PR where the issue was introduced - #8358

@vvivekiyer vvivekiyer marked this pull request as ready for review April 20, 2022 05:28
@vvivekiyer
Copy link
Contributor Author

@siddharthteotia @mcvsubbu please review.

Currently, SegmentStatusChecker initializes an object of type
SimpleHttpConnectionManager. Further down the stack, we use this object to
issue parallel HTTP GET requests to multiple servers containing segments for
the table (in MultiGetRequest). However, SimpleHttpConnectionManager is not
thread safe. So we should be using an instance of MultithreadedHttpConnectionManager.

Added a TODO to improve MultiGetRequest.java to only work on HttpConnectionManager
object that is an instanceOf MultithreadedHttpConnectionManager.
@vvivekiyer vvivekiyer force-pushed the http_connection_manager branch from fb26ceb to 5c86463 Compare April 20, 2022 05:35
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2022

Codecov Report

Merging #8568 (5c86463) into master (829ede9) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #8568      +/-   ##
============================================
- Coverage     70.83%   70.81%   -0.03%     
- Complexity     4291     4307      +16     
============================================
  Files          1686     1688       +2     
  Lines         88226    88295      +69     
  Branches      13351    13358       +7     
============================================
+ Hits          62492    62522      +30     
- Misses        21362    21408      +46     
+ Partials       4372     4365       -7     
Flag Coverage Δ
integration1 27.29% <100.00%> (-0.15%) ⬇️
integration2 25.90% <100.00%> (-0.03%) ⬇️
unittests1 67.11% <ø> (+0.10%) ⬆️
unittests2 14.13% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
.../org/apache/pinot/common/http/MultiGetRequest.java 100.00% <ø> (ø)
...e/pinot/controller/helix/SegmentStatusChecker.java 86.16% <100.00%> (ø)
...data/manager/realtime/DefaultSegmentCommitter.java 0.00% <0.00%> (-80.00%) ⬇️
...ller/helix/core/minion/TaskTypeMetricsUpdater.java 80.00% <0.00%> (-20.00%) ⬇️
...er/api/resources/LLCSegmentCompletionHandlers.java 43.56% <0.00%> (-18.82%) ⬇️
...data/manager/realtime/SegmentCommitterFactory.java 88.23% <0.00%> (-11.77%) ⬇️
...al/segment/index/readers/RangeIndexReaderImpl.java 80.21% <0.00%> (-8.81%) ⬇️
...altime/ServerSegmentCompletionProtocolHandler.java 51.42% <0.00%> (-6.67%) ⬇️
...roller/helix/core/relocation/SegmentRelocator.java 72.97% <0.00%> (-5.41%) ⬇️
...r/validation/RealtimeSegmentValidationManager.java 71.42% <0.00%> (-2.86%) ⬇️
... and 30 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 829ede9...5c86463. Read the comment docs.

@siddharthteotia siddharthteotia merged commit a08f8c6 into apache:master Apr 20, 2022
@vvivekiyer vvivekiyer deleted the http_connection_manager branch August 10, 2022 20:41
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.

3 participants