anshul98ks123 opened a new pull request, #19716:
URL: https://github.com/apache/pinot/pull/19716
Suggested labels: `bugfix`, `observability`, `release-notes` (new controller
config key; error text changes)
## Problem
A controller API that fails usually knows why, but only the controller log
hears it. The error response carries the message of the outermost exception,
and that is often just a wrapper.
For example, upload a CSV through `/ingestFromFile` where a single-value
column holds a `;`, the CSV reader's default multi-value delimiter. The
response is:
```
{"code":500,"error":"Caught exception when ingesting file into table:
foo_OFFLINE. Caught exception while reading data"}
```
The root cause, `Cannot read single-value from Object[]: [a, b] for column:
name`, is logged and dropped. Neither the caller nor any tool built on the API
can say which column or which value failed.
This is not specific to ingestion:
- `ControllerApplicationException` calls `super(message, status)`, so it
passes its throwable to the logger only. 199 call sites hand it a cause that
never reaches a response.
- 32 more sites build the message from `e.getMessage()` but never pass `e`.
- A throwable that isn't a `WebApplicationException` reaches
`WebApplicationExceptionMapper`, which answers with only the outermost
`t.getMessage()`.
## Change
The fix is where every error body is built, so no call site has to change
how it reports failures.
- **`ExceptionUtils.appendCauses(message, cause, maxCauses,
maxCauseLength)`** (pinot-common) appends the cause chain's messages to a
message, each after ` -> `. It is bounded and doesn't repeat what is already
said:
- A cause whose message already appears as a whole in the text emitted so
far is skipped. Examples: the `"…: " + e.getMessage()` idiom, a wrapper that
repeats its cause, and the `cause.toString()` message that `new
RuntimeException(cause)` generates.
- Repeats are dropped before the bound applies. At most 5 causes are kept:
the outermost and the innermost 4, with `...` between them. The root cause is
never dropped.
- Each cause is abbreviated **in the middle** to 1024 characters, so a
trailing `for column: X` survives a long value dump. Surrogate pairs are never
split.
- A root cause without a message is named by its simple class name. Cycles
terminate, and suppressed exceptions are ignored.
- **`ControllerApplicationException`** keeps the throwable as its cause in
`ExceptionLogMode.FULL`, the default for the 4-arg constructors. `getMessage()`
is unchanged.
- New `LOG_ONLY` logs the throwable but does not keep it.
- Like `TYPE_ONLY` (#19238), it keeps the causes out of the response.
- A 4xx whose message already contains `e.getMessage()` now logs that
message once instead of `"<msg> exception: <msg>"`.
- **`WebApplicationExceptionMapper`** answers with
`appendCauses(t.getMessage(), t.getCause(), 5, 1024)`. This covers
`ControllerApplicationException`, other `WebApplicationException`s (e.g.
Jersey's `ParamException` with its `NumberFormatException`) and unexpected
throwables. An unexpected throwable without a message is named by its type.
`code` is unchanged.
- **`controller.api.error.response.include.causes`** (default `true`). Set
it to `false` to answer with the message alone, byte for byte as before. A test
flips it on a running controller.
- The 32 sites that dropped their cause now pass it. In total, 223 call
sites now keep a cause for the response.
### What keeps its causes out of the response
Some causes describe something the caller must not learn, so these paths use
`LOG_ONLY`:
- **Permission checks, before the caller is authorized:**
`AccessControlUtils.validatePermission`, `FineGrainedAuthUtils` (whose
throwable is already logged), and the table-access check in segment download.
The causes come from the access control plugin and can name an IdP, internal
hosts or classpath conflicts.
- **Segment upload catch-alls** (single, re-ingested and batch): a segment
can be fetched from a caller-supplied `DOWNLOAD_URI`. Specific failures thrown
inside, such as invalid segment metadata, still carry their causes.
- **`/ingestFromFile` failures other than record failures.**
- A batch config can make a record reader open a controller-local file
(e.g. `recordReader.prop.descriptorFile`), so a setup failure's causes could
reveal whether that file exists.
- The new `RecordProcessingException` (pinot-segment-spi) marks record
failures at the two row-level wrap sites, in the stats pass and the indexing
pass. Only those failures keep their cause.
- `RecordProcessingException` is a `RuntimeException`, and the wrapper
messages are unchanged.
- **`/ingestFromURI`** keeps its generic responses (`TYPE_ONLY`, #19238).
For the upload above, the response now reads:
```
Caught exception when ingesting file into table: foo_OFFLINE. Caught
exception while reading data -> Caught exception while transforming data type
for column: name -> Cannot read single-value from Object[]: [cooper, max] for
column: name
```
## Disclosure, and the default
Beyond the paths above, callers of every controller endpoint now see the
deeper causes of failures. Many sites already put `e.getMessage()` in the
response.
What I checked:
- **Access-control users:** causes come from ZK and user-config parsing.
Jackson 2.22 leaves `INCLUDE_SOURCE_IN_LOCATION` off, so parse errors don't
echo the request body.
- **Log download:** it validates the path under the log root and already
reports a missing file.
- **Page-cache warmup:** it answers with fixed messages.
- **Record failures from Parquet or ORC readers:** these can name the
staging copy of the uploaded file under the controller's temp dir.
The independent review preferred making this opt-in per call site, or
default-off. I kept it on by default because the point is that the root cause
reaches callers without every site having to opt in. The kill switch restores
the previous bodies fleet-wide, and any site can opt out with `LOG_ONLY` or
`TYPE_ONLY`. I'm happy to flip the default if reviewers prefer.
## Tests
- `ExceptionUtilsTest` (38):
- de-dup against the emitted text, whole-word matching only (`"5"` is not
already said by `"table_5"`), and repeats dropped before the bound;
- a middle abbreviation that keeps the tail;
- blank and generated messages;
- bounds, including `maxCauses = 1` keeping the root;
- surrogate pairs and cycles.
- `WebApplicationExceptionMapperTest` (12):
- with and without a cause;
- `LOG_ONLY` and `TYPE_ONLY`;
- Jersey-style `WebApplicationException`, and unexpected throwables with
and without messages;
- bounds;
- the config key, including byte-for-byte old bodies with it off.
- `ControllerApplicationExceptionTest` (+7): which modes keep the cause;
logging for 4xx and 5xx.
- `PinotIngestionRestletResourceTest` (9): record failures keep their cause
and setup failures don't, on the 400 and 500 branches; cyclic chains; bounds.
- `RecordProcessingExceptionTest` (2): both row-level passes throw the
marker.
- `AccessControlUtilsTest`, `FineGrainedAuthUtilsTest`: a throwing plugin's
failure does not become the cause.
- `PinotIngestionRestletResourceStatelessTest` (+4), against a real
controller over HTTP:
- the exact body above;
- the old body with the kill switch off;
- the illegal-argument 400 unchanged;
- `/ingestFromURI` still generic.
- The full `pinot-controller` suite passes. No integration test compares an
error body exactly; they all check with `contains`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
