fix(runner/pinot): query the broker /query/sql path with controller fallback - #529
fix(runner/pinot): query the broker /query/sql path with controller fallback#529mayankpande88 wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Pinot client to query the broker path (/query/sql) first and fall back to the controller path (/sql) upon receiving a 404 status code, allowing it to work with both broker and controller URLs. A new doRaw helper is introduced to return raw HTTP responses and status codes, and tests are updated to verify this fallback behavior. Feedback was provided regarding a potential data race in the new test due to unsynchronized access to a slice across goroutines, suggesting the use of a channel instead.
| var paths []string | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| paths = append(paths, r.URL.Path) | ||
| if r.URL.Path == "/query/sql" { | ||
| w.WriteHeader(http.StatusNotFound) | ||
| _, _ = w.Write([]byte(`not found`)) | ||
| return | ||
| } | ||
| _, _ = w.Write([]byte(`{"resultTable":{}}`)) | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| c := New(srv.URL, &http.Client{Timeout: 5 * time.Second}) | ||
| if _, err := c.Query(context.Background(), "SELECT 1"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| want := []string{"/query/sql", "/sql"} | ||
| if len(paths) != 2 || paths[0] != want[0] || paths[1] != want[1] { | ||
| t.Errorf("request paths = %v; want %v", paths, want) | ||
| } |
There was a problem hiding this comment.
The test TestQuery_FallsBackToControllerOn404 accesses the paths slice from both the HTTP server handler goroutine and the main test goroutine without explicit synchronization. Although there is a logical happens-before relationship via the HTTP round-trip, the Go race detector may flag this as a data race under -race because there is no explicit synchronization. Using a buffered channel to collect the request paths is a cleaner, safer, and idiomatic way to avoid any potential data races in tests.
pathsChan := make(chan string, 2)
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
pathsChan <- r.URL.Path
if r.URL.Path == "/query/sql" {
w.WriteHeader(http.StatusNotFound)
_, _ = w.Write([]byte("not found"))
return
}
_, _ = w.Write([]byte("{\"resultTable\":{}}"))
}))
defer srv.Close()
c := New(srv.URL, &http.Client{Timeout: 5 * time.Second})
if _, err := c.Query(context.Background(), "SELECT 1"); err != nil {
t.Fatal(err)
}
close(pathsChan)
var paths []string
for p := range pathsChan {
paths = append(paths, p)
}
want := []string{"/query/sql", "/sql"}
if len(paths) != 2 || paths[0] != want[0] || paths[1] != want[1] {
t.Errorf("request paths = %v; want %v", paths, want)
}
ec9afd8 to
1d52cf6
Compare
…allback pinot_query always POSTed to /sql, but the chart directs operators to point PINOT_URL at the Pinot broker, whose query path is /query/sql — so every query 404'd on a broker-targeted PINOT_URL. POST to /query/sql first (the documented broker setup) and fall back to /sql on a 404, so both broker and controller URLs work. The /tables and /schemas endpoints stay controller-only, as documented. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2HAXQH1gEtX7h4sUJsi9j (cherry picked from commit 9a8ba0d)
…404s A 404 from a missing table otherwise reads identically to a misconfigured PINOT_URL.
|
Rebased onto Worth recording for review: unlike the rest of this batch, this is not a legacy-parity fix — the legacy agent has no Pinot support at all (no Also improved the error when both paths 404, so a missing table doesn't read as a misconfigured URL. |
017b7a9 to
719254b
Compare
|
📦 Image Tags Updated |
1 similar comment
|
📦 Image Tags Updated |
3 participants
pinot_query always POSTed to /sql, but the chart directs operators to
point PINOT_URL at the Pinot broker, whose query path is /query/sql —
so every query 404'd on a broker-targeted PINOT_URL.
POST to /query/sql first (the documented broker setup) and fall back to
/sql on a 404, so both broker and controller URLs work. The /tables and
/schemas endpoints stay controller-only, as documented.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01L2HAXQH1gEtX7h4sUJsi9j