Skip to content

Changed deprecated BrokerClient#17846

Merged
kfaraz merged 8 commits intoapache:masterfrom
akshatmardia:deprecated-BrokerClient
Apr 4, 2025
Merged

Changed deprecated BrokerClient#17846
kfaraz merged 8 commits intoapache:masterfrom
akshatmardia:deprecated-BrokerClient

Conversation

@akshatmardia
Copy link
Copy Markdown
Contributor

@akshatmardia akshatmardia commented Mar 29, 2025

Fixes #17435.

Description

Changed the deprecated BrokerClient to the new one

Release note

Key changed/added classes in this PR
  • changed org.apache.druid.discovery.BrokerClient to org.apache.druid.sql.client.BrokerClient and updated how it is used

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@github-actions github-actions Bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Mar 29, 2025
@akshatmardia akshatmardia marked this pull request as draft March 30, 2025 00:55
@akshatmardia akshatmardia marked this pull request as ready for review March 30, 2025 05:03
kfaraz
kfaraz requested changes Mar 30, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

Please use BrokerClient and not the impl class BrokerClientImpl.
Please build Druid and test out your changes before marking your PR ready for review.

The current set of changes is sure to break compilation since you haven't changed any of the call sites that create a SegmentLoadStatusFetcher.

@akshatmardia akshatmardia marked this pull request as draft March 30, 2025 17:25
@akshatmardia
Copy link
Copy Markdown
Contributor Author

akshatmardia commented Mar 30, 2025 •
edited
Loading

Thank you for the feedback I sort of overlooked some issues but I'll try to fix them. Apologies for that.

@akshatmardia
Copy link
Copy Markdown
Contributor Author

akshatmardia commented Mar 31, 2025

@kfaraz Build is now successful and tests are passing. I need some help with testing it on a cluster so any guidance would be appreciated.

@akshatmardia akshatmardia requested a review from kfaraz April 1, 2025 02:23
@akshatmardia
Copy link
Copy Markdown
Contributor Author

akshatmardia commented Apr 1, 2025

I am aware of the failing static check I will fix it by tomorrow.

kfaraz
kfaraz requested changes Apr 2, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

The current implementation is not correct since it uses the /sql/task endpoint instead of /sql.

@kfaraz kfaraz marked this pull request as ready for review April 3, 2025 02:43
kfaraz
kfaraz reviewed Apr 3, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix, @akshatmardia !
I have left some final suggestions.

Comment thread ...core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/SegmentLoadStatusFetcher.java Outdated
Comment thread ...core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/SegmentLoadStatusFetcher.java Outdated
Comment thread server/src/main/java/org/apache/druid/client/broker/BrokerClientImpl.java Outdated
Comment thread server/src/main/java/org/apache/druid/client/broker/BrokerClientImpl.java Outdated
Comment thread .../multi-stage-query/src/test/java/org/apache/druid/msq/exec/SegmentLoadStatusFetcherTest.java Outdated
@akshatmardia
Copy link
Copy Markdown
Contributor Author

akshatmardia commented Apr 3, 2025 •
edited
Loading

@kfaraz Appreciate the suggestions. I've pushed the changes but I'm confused as to why the build is failing.

@kfaraz
Copy link
Copy Markdown
Contributor

kfaraz commented Apr 3, 2025

@akshatmardia , the build failure is unrelated to your changes.
Could you also delete the deprecated BrokerClient and its test usages too?

kfaraz
kfaraz reviewed Apr 3, 2025
View reviewed changes
Comment thread extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestBase.java Outdated
@kfaraz kfaraz merged commit 252cee5 into apache:master Apr 4, 2025
74 of 75 checks passed
@kfaraz
Copy link
Copy Markdown
Contributor

kfaraz commented Apr 4, 2025

Merged since failure was unrelated and is being tackled in a separate PR #17867

@kfaraz kfaraz mentioned this pull request Apr 4, 2025
Closed
10 tasks
gianm added a commit to gianm/druid that referenced this pull request May 8, 2025
@gianm
Fix resultFormat of SegmentLoadStatusFetcher.
dca8ea1 
In apache#17846, SegmentLoadStatusFetcher was changed to send a resultFormat
of "text/plain" rather than "objectLines". This effectively broke the
feature of waiting for load status, since the load status would always
be FAILED.
@gianm gianm mentioned this pull request May 8, 2025
Merged
cryptoe pushed a commit that referenced this pull request May 8, 2025
@gianm
Fix resultFormat of SegmentLoadStatusFetcher. (#17987)
04ad5c5 
In #17846, SegmentLoadStatusFetcher was changed to send a resultFormat
of "text/plain" rather than "objectLines". This effectively broke the
feature of waiting for load status, since the load status would always
be FAILED.
@capistrant capistrant added this to the 34.0.0 milestone Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfaraz kfaraz kfaraz approved these changes

Assignees

No one assigned

Labels

Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262

Projects

None yet

Milestone

34.0.0

Development

Successfully merging this pull request may close these issues.

Remove org.apache.druid.discovery.BrokerClient by switching to org.apache.druid.sql.client.BrokerClient

3 participants

@akshatmardia @kfaraz @capistrant