Skip to content

Conversation

@xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Aug 8, 2025

This PR introduces a query option: ignoreMissingSegments to ignore server-side missing segment errors and an optional broker dynamic config (pinot.broker.query.ignore.missing.segments) to set it automatically.

Key changes:

  • New QueryOptionKey: ignoreMissingSegments
  • New Broker config: pinot.broker.query.ignore.missing.segments (default false)
  • SSE: broker sets option and filters SERVER_SEGMENT_MISSING in reduce
  • MSE: broker sets option and skips adding missing-segment exceptions
  • Server: respects option to not populate SERVER_SEGMENT_MISSING
  • Tests: QueryOptionsUtils parsing, BrokerReduceService behavior, server query exceptions test.

This alleviates transient failures when a segment is deleted but the routing table is not yet updated.

@xiangfu0 xiangfu0 added enhancement Configuration Config changes (addition/deletion/change in behavior) feature labels Aug 8, 2025

This comment was marked as outdated.

@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2025

Codecov Report

❌ Patch coverage is 24.13793% with 22 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@44dd108). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...requesthandler/MultiStageBrokerRequestHandler.java 13.33% 13 Missing ⚠️
...sthandler/BaseSingleStageBrokerRequestHandler.java 40.00% 2 Missing and 1 partial ⚠️
...e/pinot/core/query/reduce/BrokerReduceService.java 25.00% 2 Missing and 1 partial ⚠️
...inot/core/query/reduce/StreamingReduceService.java 33.33% 0 Missing and 2 partials ⚠️
...core/query/executor/ServerQueryExecutorV1Impl.java 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master   #16556   +/-   ##
=========================================
  Coverage          ?   63.31%           
  Complexity        ?     1379           
=========================================
  Files             ?     3027           
  Lines             ?   176446           
  Branches          ?    27074           
=========================================
  Hits              ?   111717           
  Misses            ?    56164           
  Partials          ?     8565           
Flag Coverage Δ
custom-integration1 100.00% <ø> (?)
integration 100.00% <ø> (?)
integration1 100.00% <ø> (?)
integration2 0.00% <ø> (?)
java-11 63.27% <24.13%> (?)
java-21 63.28% <24.13%> (?)
temurin 63.31% <24.13%> (?)
unittests 63.31% <24.13%> (?)
unittests1 56.45% <33.33%> (?)
unittests2 33.14% <13.79%> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xiangfu0
Copy link
Contributor Author

xiangfu0 commented Aug 8, 2025

Addressed Codecov by adding a streaming reduce unit test to verify ignoreMissingSegments filtering in gRPC path. Also fixed the test to use a selection query per StreamingReduceService (selection-only).

@xiangfu0
Copy link
Contributor Author

xiangfu0 commented Aug 8, 2025

Addressed review feedback: simplified broker config check in BaseSingleStageBrokerRequestHandler and extracted a helper to avoid duplication. No functional changes.

@xiangfu0 xiangfu0 force-pushed the adding-ignore-missing-segments-query-option branch 2 times, most recently from a336bda to 3ed8d9f Compare August 8, 2025 20:11
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

We already have a config pinot.server.instance.table.deleted.segments.cache.ttl.minutes to control how long to keep the deleted segments (not count them as missing). See BaseTableDataManager._recentlyDeletedSegments for more details.
Do you still see this new option necessary?

@xiangfu0
Copy link
Contributor Author

We already have a config pinot.server.instance.table.deleted.segments.cache.ttl.minutes to control how long to keep the deleted segments (not count them as missing). See BaseTableDataManager._recentlyDeletedSegments for more details. Do you still see this new option necessary?

The caveat for _recentlyDeletedSegments cache is the server may still restart and clear the cache.

@xiangfu0 xiangfu0 force-pushed the adding-ignore-missing-segments-query-option branch 2 times, most recently from c0bdb98 to 0172726 Compare August 17, 2025 22:51
…fig; skip SERVER_SEGMENT_MISSING errors when enabled; add SSE/MSE wiring and unit tests
@xiangfu0 xiangfu0 force-pushed the adding-ignore-missing-segments-query-option branch from 0172726 to 32b9441 Compare August 17, 2025 23:07
@xiangfu0 xiangfu0 requested a review from Copilot August 18, 2025 06:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new query option ignoreMissingSegments and corresponding broker configuration to handle transient segment availability issues. The feature allows queries to continue executing when segments are temporarily missing due to routing table lag after segment deletion.

  • Adds ignoreMissingSegments query option to filter out SERVER_SEGMENT_MISSING exceptions
  • Introduces broker configuration pinot.broker.query.ignore.missing.segments to automatically enable the option
  • Implements filtering logic in both single-stage and multi-stage query execution paths

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
CommonConstants.java Defines new broker config and query option constants
QueryOptionsUtils.java Adds utility method to parse the new query option
QueryOptionsUtilsTest.java Tests for the new query option parsing logic
ServerQueryExecutorV1Impl.java Server-side logic to skip populating missing segment exceptions when option is set
BrokerReduceService.java Broker-side filtering of missing segment exceptions for single-stage queries
StreamingReduceService.java Broker-side filtering of missing segment exceptions for streaming queries
BaseSingleStageBrokerRequestHandler.java Auto-configuration of the query option based on broker settings
MultiStageBrokerRequestHandler.java Auto-configuration and missing segment handling for multi-stage queries
BrokerReduceServiceTest.java Tests verifying exception filtering behavior
StreamingReduceServiceTest.java Tests for streaming query exception filtering
QueryExecutorExceptionsTest.java Server-side tests for the ignore option behavior
Comments suppressed due to low confidence (1)

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:463

  • Unused variable tableNames should be removed as it's assigned but never used in this method.
        () -> query.explain(requestId, fragmentToPlanNode), timer);

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@xiangfu0 xiangfu0 merged commit 285a18b into apache:master Aug 18, 2025
17 of 18 checks passed
@xiangfu0 xiangfu0 deleted the adding-ignore-missing-segments-query-option branch August 18, 2025 07:51
@priyen-stripe
Copy link
Contributor

This PR introduces a query option: ignoreMissingSegments to ignore server-side missing segment errors and an optional broker dynamic config (pinot.broker.query.ignore.missing.segments) to set it automatically.

Key changes:

  • New QueryOptionKey: ignoreMissingSegments
  • New Broker config: pinot.broker.query.ignore.missing.segments (default false)
  • SSE: broker sets option and filters SERVER_SEGMENT_MISSING in reduce
  • MSE: broker sets option and skips adding missing-segment exceptions
  • Server: respects option to not populate SERVER_SEGMENT_MISSING
  • Tests: QueryOptionsUtils parsing, BrokerReduceService behavior, server query exceptions test.

This alleviates transient failures when a segment is deleted but the routing table is not yet updated.

@Jackie-Jiang , do you know how transient these failures typically are? how did you learn/what motivated this PR?

@xiangfu0
Copy link
Contributor Author

This PR introduces a query option: ignoreMissingSegments to ignore server-side missing segment errors and an optional broker dynamic config (pinot.broker.query.ignore.missing.segments) to set it automatically.
Key changes:

  • New QueryOptionKey: ignoreMissingSegments
  • New Broker config: pinot.broker.query.ignore.missing.segments (default false)
  • SSE: broker sets option and filters SERVER_SEGMENT_MISSING in reduce
  • MSE: broker sets option and skips adding missing-segment exceptions
  • Server: respects option to not populate SERVER_SEGMENT_MISSING
  • Tests: QueryOptionsUtils parsing, BrokerReduceService behavior, server query exceptions test.

This alleviates transient failures when a segment is deleted but the routing table is not yet updated.

@Jackie-Jiang , do you know how transient these failures typically are? how did you learn/what motivated this PR?

For large table with many segments(especially table with many kafka partitions and high retention frequency), the routing table rebuild may take longer time like couple minutes. Then queries to those servers will fail find segments.

E.g. pinot.server.instance.table.deleted.segments.cache.ttl.minutes is default to 2minutes, but the caveat is it won't prevent server restart and local cache got cleaned.

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

Labels

Configuration Config changes (addition/deletion/change in behavior) enhancement feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants