You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shauryachats (via GitHub)" <gi...@apache.org> on 2025/07/28 23:40:04 UTC
[PR] [refactor] Switching to RoutingManager for broker request handlers [pinot]
shauryachats opened a new pull request, #16442:
URL: https://github.com/apache/pinot/pull/16442
This PR updates broker request handlers to use the `RoutingManager` interface instead of `BrokerRoutingManager` to accommodate other RoutingManagers and make the codebase more modular.
It also adds `getQueryTimeoutMs` to `RoutingManager` to fetch the query timeout per table, since it is also present in the `RoutingEntry`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [refactor] Switching to RoutingManager for broker request handlers [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana merged PR #16442:
URL: https://github.com/apache/pinot/pull/16442
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [refactor] Switching to RoutingManager for broker request handlers [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #16442:
URL: https://github.com/apache/pinot/pull/16442#discussion_r2249318490
##########
pinot-core/src/main/java/org/apache/pinot/core/routing/RoutingManager.java:
##########
@@ -117,4 +117,11 @@ default boolean isTableDisabled(String tableNameWithType) {
*/
@Nullable
Set<String> getServingInstances(String tableNameWithType);
+
+ /**
+ * Returns the query timeout in milliseconds for the given table.
+ * Returns {@code null} if the table does not have a query timeout set.
+ */
+ @Nullable
+ Long getQueryTimeoutMs(String tableNameWithType);
Review Comment:
query timeout should be associated with something like a `RequestContext`. Can we avoid this method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [refactor] Switching to RoutingManager for broker request handlers [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #16442:
URL: https://github.com/apache/pinot/pull/16442#discussion_r2252809347
##########
pinot-core/src/main/java/org/apache/pinot/core/routing/RoutingManager.java:
##########
@@ -117,4 +117,11 @@ default boolean isTableDisabled(String tableNameWithType) {
*/
@Nullable
Set<String> getServingInstances(String tableNameWithType);
+
+ /**
+ * Returns the query timeout in milliseconds for the given table.
+ * Returns {@code null} if the table does not have a query timeout set.
+ */
+ @Nullable
+ Long getQueryTimeoutMs(String tableNameWithType);
Review Comment:
Makes sense. Removed the dependency of `routingManager` for retrieving query timeout for a table through the `TableConfig` by calling it directly in `BaseSingleStageBrokerRequestHandler`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [refactor] Switching to RoutingManager for broker request handlers [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #16442:
URL: https://github.com/apache/pinot/pull/16442#discussion_r2254834511
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -1945,10 +1945,12 @@ private long setQueryTimeout(String tableNameWithType, Long logicalTableQueryTim
// Use query-level timeout if exists
queryTimeoutMs = queryLevelTimeoutMs;
} else {
- Long tableLevelTimeoutMs = _routingManager.getQueryTimeoutMs(tableNameWithType);
- if (tableLevelTimeoutMs != null) {
+ TableConfig tableConfig = _tableCache.getTableConfig(tableNameWithType);
Review Comment:
before we were getting timeout from RoutingEntry, which was getting created async and was getting the timeout from Table Config retrieved from ZKMetadataProvider.
Now we are getting it from tableCache synchronously in the query path. I suppose this should be fine, but just leaving a note here to call it out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [refactor] Switching to RoutingManager for broker request handlers [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16442:
URL: https://github.com/apache/pinot/pull/16442#issuecomment-3130199496
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16442?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 56.27%. Comparing base ([`1a476de`](https://app.codecov.io/gh/apache/pinot/commit/1a476dea72ebf1c15e14ded28ae60c385f71b2c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c2b4458`](https://app.codecov.io/gh/apache/pinot/commit/c2b44586b1975bacaad1b1a9c63a4d7876a38644?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 520 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (1a476de) and HEAD (c2b4458). Click for more details.
>
> <details><summary>HEAD has 68 uploads less than BASE</summary>
>
>| Flag | BASE (1a476de) | HEAD (c2b4458) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|1|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|unittests2|3|0|
>|skip-bytebuffers-false|8|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16442 +/- ##
============================================
- Coverage 62.90% 56.27% -6.64%
+ Complexity 1386 699 -687
============================================
Files 2867 2350 -517
Lines 163354 129490 -33864
Branches 24952 20590 -4362
============================================
- Hits 102755 72867 -29888
+ Misses 52847 50610 -2237
+ Partials 7752 6013 -1739
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.27% <ø> (-6.55%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.27% <ø> (-6.64%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.27% <ø> (-6.63%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.27% <ø> (+0.45%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16442?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org