feat: add support for specifying queryable historical tiers via query context#19537
feat: add support for specifying queryable historical tiers via query context#19537jtuglu1 wants to merge 2 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
f00a25a to
121ada6
Compare
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 9 of 9 changed files.
This is an automated review by Codex GPT-5.5
8bf1431 to
dd0e846
Compare
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 9 of 9 changed files.
This is an automated review by Codex GPT-5.5
dd0e846 to
33bee46
Compare
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 9 of 9 changed files.
This is an automated review by Codex GPT-5.5
abhishekrb19
left a comment
abhishekrb19
left a comment
There was a problem hiding this comment.
LGTM. Left some comments - this is on the query path, so it'll be good to remove cleanup any redundant ops - thanks.
| @@ -604,10 +611,15 @@ private SortedMap<DruidServer, List<SegmentDescriptor>> groupSegmentsByServer( | |||
| CloneQueryMode cloneQueryMode | |||
| ) | |||
| { | |||
| final Set<String> queryableHistoricalTiers = query.context().getQueryableHistoricalTiers(); | |||
There was a problem hiding this comment.
Similar to cloneQueryMode, it'll be good to get the historical tiers once and pass it to both computeResultLevelCachingEtag() and pick() to avoid getting the tiers from parsing the context unnecessarily
| for (final int priority : queryableServers.keySet()) { | ||
| final Set<QueryableDruidServer> priorityServers = new HashSet<>(); | ||
| for (final QueryableDruidServer server : queryableServers.get(priority)) { |
There was a problem hiding this comment.
Since this is a tree map, you could avoid the double lookup by iterating on the queryableServers.int2ObjectEntrySet() once as you go instead of getting the priority and then looking it up again with it
Would likely require a small restructure here.
| ) | ||
| { | ||
| Hasher hasher = Hashing.sha1().newHasher(); | ||
| final Hasher hasher = Hashing.sha1().newHasher(); |
fca81e3 to
49c83dc
Compare
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 9 of 9 changed files.
Found one correctness issue: virtual tier aliases are compared directly with physical server tiers and can silently select no servers.
This is an automated review by Codex GPT-5.6-Sol
| for (final Int2ObjectMap.Entry<Set<QueryableDruidServer>> entry : queryableServers.int2ObjectEntrySet()) { | ||
| final Set<QueryableDruidServer> priorityServers = new HashSet<>(); | ||
| for (final QueryableDruidServer server : entry.getValue()) { | ||
| if (queryableHistoricalTiers.contains(server.getServer().getTier())) { |
There was a problem hiding this comment.
[P1] Resolve virtual tier aliases before filtering servers
This compares requested tiers directly with each server's physical tier, but the PR description says queryableHistoricalTiers supports virtual tiers from historicalTierAliases. Alias keys are never physical tiers: Coordinator expansion loads segments only onto their mapped tiers. Therefore, with historicalTierAliases={"hot":["hot_1","hot_2"]}, querying queryableHistoricalTiers=["hot"] matches no server and silently returns empty or partial results. The Broker's coordinator-config view should expand aliases before hasQueryableHistoricalServer and getQueryableHistoricalServers perform these checks, with coverage for virtual and mixed virtual/physical tier sets.
4 participants
Description
Support specifying specific historical tiers to be queried dynamically via query context. Tiers may either be physical or virtual (mapped to physical tiers via coordinator dynamic config).
Release note
Support specifying specific historical tiers to be queried dynamically via query context.
This PR has: