shivam-startree opened a new pull request, #19454: URL: https://github.com/apache/pinot/pull/19454
## Summary Tags the pre-processing record emitted by `QueryLogger.logQueryReceived` with a `QUERY_RECEIVED` SLF4J marker. ## Motivation `QueryLogger` emits two records per query **from the same logger**: the pre-processing `SQL query for request <id>:` line, and the completion line carrying the stats. An operator who wants to keep one but not the other has no clean way to express that today. - **Logger name** cannot separate them — there is a single `getLogger(QueryLogger.class)`. - **Message text** can, but a log4j2 filter keyed on wording is brittle (it breaks the moment the message is reworded) and is evaluated per event. A marker separates them declaratively at no per-event cost: ```xml <MarkerFilter marker="QUERY_RECEIVED" onMatch="DENY" onMismatch="NEUTRAL"/> ``` or the record can be routed to its own appender — keeping it on the broker's disk for forensics while keeping it off a shipped log stream. That is the case we have: the pre-processing record is valuable to retain locally but accounts for a large share of shipped log volume. ## Consistency with existing code This follows the convention already used in `pinot-broker` rather than introducing a new one: - `MultiStageBrokerRequestHandler` declares `MSE_STATS_MARKER` - `PinotClientRequest` declares `QUERY_RESPONSE_EXCEPTION` ## Backward compatibility No behaviour change, no new configuration, and the record **stays enabled by default**. `logQueryReceived` exists to keep a trace of queries that never succeed — `logQueryCompleted` only fires on completion, so for a query that hangs, times out without a response, or kills the broker, the pre-processing record is the only evidence. `pinot.broker.query.log.logBeforeProcessing` (#16056) already covers switching it off wholesale; this adds the finer-grained option of keeping it while controlling where it goes. ## Note for reviewers — a trap worth knowing about The existing test stubbed `info(String, Object, Object)`. `logQueryReceived` now calls the `Marker` overload, so the stub had to be extended to cover it. **Without that change `shouldLogQueryReceivedWhenAllowed` silently observes no log record and fails** — worth watching for if similar markers are added elsewhere. Adds a test asserting the received record carries the marker and the completion record does not. ## Note on verification I could not build locally — `master` now requires JDK 25 and only 21 was available here, and the `-Djdk.version=21` fallback fails on dependency class-file versions. Relying on CI for compile and test; opened as a draft for that reason. ## Related Companion draft collapses newlines in the logged query (79.4% of one broker's log lines). Independent of this change. -- 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]
