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.

Sign up for GitHub

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

Jump to bottom

Refactor DirectDruidClient, extract QueryContextInputStreamResponseHandler #8848

Closed

zhenxiao wants to merge 9 commits into apache:master from zhenxiao:direct-client

Contributor

zhenxiao commented Nov 9, 2019

Fixes #8444
CC @leventov

leventov reviewed

View reviewed changes

server/src/main/java/org/apache/druid/client/BufferInputStreamResponseHandler.java Outdated

    
              import java.util.concurrent.atomic.AtomicLong;

              import java.util.concurrent.atomic.AtomicReference;

              public class BufferInputStreamResponseHandler implements HttpResponseHandler<InputStream, InputStream>

Member

leventov Nov 16, 2019

Please add a class-level Javadoc explaining something about this class. Also, its name suggests that it could possibly be emulated by combining existing pieces (like BufferedInputStream + some other HttpResponseHandler). If that is not possible, or the semantics are actually different, it should be highlighted in the comment.

Optional: explain the concurrency (the class has a lot of it: Atomic fields, BlockingQueue, etc.) and add concurrent access documentation to the methods of this class.

zhenxiao changed the title ~~Refactor DirectDruidClient, extract BufferInputStreamResponseHandler~~ Refactor DirectDruidClient, extract QueryContextInputStreamResponseHandler

Contributor Author

zhenxiao commented Nov 17, 2019

thank you, @leventov
get comments addressed

leventov requested changes

View reviewed changes

server/src/main/java/org/apache/druid/client/QueryContextInputStreamResponseHandler.java Outdated

    
              /**

               * An HTTP response handler which uses sequence input streams to create a final InputStream.

               *

               * This implementation takes query context into consideration, e.g. timeout, maxQueuedBytes, backpressue.

Member

leventov Nov 17, 2019

Typo, "backpressure"

server/src/main/java/org/apache/druid/client/QueryContextInputStreamResponseHandler.java Outdated

    
               * may end with an IOException upon read()

               *

               * {@link org.apache.druid.java.util.http.client.response.SequenceInputStreamResponseHandler} also uses

               * sequence input streams to create final InputStream, but it does not have timeout, backpressue,

Member

leventov Nov 17, 2019

and here

server/src/main/java/org/apache/druid/client/QueryContextInputStreamResponseHandler.java Outdated

    
               * An HTTP response handler which uses sequence input streams to create a final InputStream.

               *

               * This implementation takes query context into consideration, e.g. timeout, maxQueuedBytes, backpressue.

               * It uses a blocking queue to feed a SequenceInputStream that is terminated whenever the handler's Done

Member

leventov Nov 17, 2019

Please make "Done method" a javadoc link

leventov reviewed

View reviewed changes

server/src/main/java/org/apache/druid/client/QueryContextInputStreamResponseHandler.java Outdated

    
               * method is called or a throwable is detected.

               *

               * The resulting InputStream will attempt to terminate normally, but on exception in HttpResponseHandler

               * may end with an IOException upon read()

Member

leventov Nov 17, 2019

And please make read() a javadoc link

Contributor Author

zhenxiao commented Nov 17, 2019

thank you, @leventov
comments addressed

leventov requested changes

View reviewed changes

server/src/main/java/org/apache/druid/client/QueryContextInputStreamResponseHandler.java Outdated

    
               * method is called or a throwable is detected.

               * This implementation takes query context into consideration, e.g. timeout, maxQueuedBytes, backpressure.

               * It uses a blocking queue to feed a SequenceInputStream that is terminated whenever the handler's

               * {@link #Done()} is called or a throwable is detected.

Member

leventov Nov 17, 2019

This link is not valid.

server/src/main/java/org/apache/druid/client/QueryContextInputStreamResponseHandler.java Outdated

    
               *

               * The resulting InputStream will attempt to terminate normally, but on exception in HttpResponseHandler

               * may end with an IOException upon read()

               * may end with an IOException upon {@link #read()}

Member

leventov Nov 17, 2019

And this

Contributor Author

zhenxiao commented Nov 17, 2019

thank you @leventov
how about this one?

leventov reviewed

View reviewed changes

Member

leventov left a comment

LGTM, please fix inspection errors

Contributor Author

zhenxiao commented Nov 24, 2019

thank you, @leventov
inspection fixed

Member

leventov commented Nov 25, 2019

CI still fails

Contributor Author

zhenxiao commented Nov 25, 2019

Hi @leventov
seems CI failures are not related to this PR. Do you think I need to merge master commits into this one?

zhenxiao force-pushed the direct-client branch from d649a8e to 390532b Compare

November 25, 2019 07:07

Member

leventov commented Dec 2, 2019

Yes, merge master

zhenxiao force-pushed the direct-client branch 5 times, most recently from 09ccafb to 30868ef Compare

December 3, 2019 07:46

Member

leventov commented Dec 3, 2019

@zhenxiao note for future: don't force push branches. merge master into PR branch, do not rebase.

Contributor Author

zhenxiao commented Dec 3, 2019 •

edited

Loading

whoops, CI pass now @leventov how about this one?
when merge master into my branch, let's say before merge my branch:
old-master -> my patch
after merge, it could be:
A. old-master -> my patch -> (old to new master patches)
or:
B. new master -> my patch

A will have conflicts with base brach. B will need force push. which one should I do?

zhenxiao force-pushed the direct-client branch from bac2ea0 to 57899a4 Compare

December 3, 2019 20:56

Member

leventov commented Dec 6, 2019

When you are at the state "old-master -> my patch", you just need to run two actions: git fetch origin master and git merge origin/master. If there are conflicts, resolve them and commit.

Then history will look like this:

old-master -> your patch -> merge commit (HEAD of your PR branch)
   \____ new master _______/

Member

leventov commented Dec 6, 2019

Inspection build errors: https://teamcity.jetbrains.com/viewLog.html?buildId=2660569&tab=Inspection&buildTypeId=OpenSourceProjects_Druid_InspectionsPullRequests

Contributor Author

zhenxiao commented Dec 6, 2019

thank you so much, @leventov I will do it next time
get inspection errors fixed. See how this goes

Contributor Author

zhenxiao commented Dec 6, 2019

tests passed. @leventov could you please take a look?

Member

leventov commented Dec 7, 2019

Inspection build still fails: https://teamcity.jetbrains.com/viewLog.html?buildId=2665308&tab=Inspection&buildTypeId=OpenSourceProjects_Druid_InspectionsPullRequests

zhenxiao added 3 commits

January 30, 2020 17:49


          Refactor DirectDruidClient, extract BufferInputStreamResponseHandler

7a1a190


          Add javadoc for QueryContextInputStreamResponseHandler

5a8fc51


          Fix javadoc

e217303

zhenxiao added 5 commits

January 30, 2020 17:49


          Fix javadoc

5067add


          Fix inspection error

44244fe


          Fix Inspection and remove unused variable

8b58117


          Fix inspection error

861e99d


          replace statement lambda with expression lambda

806825f

zhenxiao force-pushed the direct-client branch from d9d0a86 to 806825f Compare

January 31, 2020 01:50

codecov-io commented Jan 31, 2020 •

edited

Loading

Codecov Report

Merging #8848 into master will increase coverage by 0.96%.
The diff coverage is 17.96%.

@@             Coverage Diff              @@
##             master    #8848      +/-   ##
============================================
+ Coverage     62.93%    63.9%   +0.96%     
- Complexity    22577    22580       +3     
============================================
  Files          2980     2865     -115     
  Lines        123590   117386    -6204     
  Branches      17030    15646    -1384     
============================================
- Hits          77785    75010    -2775     
+ Misses        38708    35510    -3198     
+ Partials       7097     6866     -231

Impacted Files	Coverage Δ	Complexity Δ
...ava/org/apache/druid/client/DirectDruidClient.java	`79.31% <100%> (+41.17%)`	`9 <0> (ø)`	⬇️
...client/QueryContextInputStreamResponseHandler.java	`16.26% <16.26%> (ø)`	`2 <2> (?)`
...java/org/apache/druid/client/BrokerServerView.java	`57.37% <50%> (+1.12%)`	`10 <1> (ø)`	⬇️
.../server/coordination/SegmentChangeRequestNoop.java	`20% <0%> (-20%)`	`1% <0%> (-1%)`
.../apache/druid/client/BatchServerInventoryView.java	`80.76% <0%> (-5.77%)`	`10% <0%> (-1%)`
...ache/druid/indexing/overlord/RemoteTaskRunner.java	`63.26% <0%> (-0.35%)`	`83% <0%> (-1%)`
...che/druid/metadata/SqlSegmentsMetadataManager.java	`71.69% <0%> (-0.27%)`	`66% <0%> (-1%)`
...nsole/src/components/show-history/show-history.tsx
web-console/src/components/loader/loader.tsx
web-console/src/components/deferred/deferred.tsx
... and 123 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 204ba99...806825f. Read the comment docs.


          Remove unused method

71e4761

stale bot commented Mar 31, 2020

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

stale bot added the stale label

stale bot commented Apr 28, 2020

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

stale bot closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels