Skip to content
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.

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

Migrated from deprecated BrokerClient to new BrokerClient #17681

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tarunk8924
Copy link

@tarunk8924 tarunk8924 commented Jan 29, 2025

Title:
Improved Segment Load Status Fetcher: Enhanced Query Handling and Code Refactoring

Description:
This PR introduces improvements and refactoring to the SegmentLoadStatusFetcher in the Apache Druid project. The changes aim to improve code maintainability, enhance performance in segment loading operations, and ensure more reliable status checks when waiting for segments to load.

Key Changes:

  1. Enhanced Query Handling:

    • Improved SQL query logic to handle additional segment scenarios, including on-demand and unknown segment types.
    • Ensured efficient filtering and robust conditional handling to better track segment loading.
  2. Status Update Logic:

    • Simplified and optimized the status update mechanism.
    • Ensured accurate updates for segment status during the wait period, including proper tracking of states (success, failure, timeout).
  3. Exception Handling Improvements:

    • Enhanced error handling during load status fetch operations.
    • Updated logic to avoid unnecessary task failures if an exception occurs during the segment wait.
  4. Refactoring for Code Readability:

    • Reorganized methods to improve readability and reduce redundant operations.
    • Refactored the version condition creation logic to minimize computational overhead.
  5. Thread Management:

    • Properly shutdown executor service to prevent resource leaks.
    • Added additional safety checks during concurrent operations.

Testing:*

  • Verified the query execution against multiple broker scenarios.
  • Conducted unit tests to ensure accurate segment status updates.
  • Stress-tested the system to confirm that timeout and exception handling worked as expected.

Backward Compatibility:
The changes maintain backward compatibility with existing segment loading workflows in Apache Druid.

Reviewer Notes:

  • Review the SQL query and version condition logic for correctness and efficiency.

  • Validate exception handling mechanisms for edge cases.

  • Ensure thread handling improvements are correctly implemented.
    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.

@kfaraz
Copy link
Contributor

kfaraz commented Jan 30, 2025

@tarunk8924 , thanks for the changes!
I think you have reformatted which has bloated up the diff making the review difficult.
Could you please take a look at the diff and revert the unnecessary changes?

@tarunk8924
Copy link
Author

tarunk8924 commented Jan 30, 2025 via email

@tarunk8924
Copy link
Author

@kfaraz I have done the changes..!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants