Jackie-Jiang commented on code in PR #17341:
URL: https://github.com/apache/pinot/pull/17341#discussion_r2604455966
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1505,6 +1505,8 @@ public static class Controller {
public static final String SEGMENT_NAME_HTTP_HEADER = "Pinot-Segment-Name";
public static final String TABLE_NAME_HTTP_HEADER = "Pinot-Table-Name";
public static final String PINOT_QUERY_ERROR_CODE_HEADER =
"X-Pinot-Error-Code";
+ public static final String PINOT_HTTP_RESPONSE_CODE_REPRESENT_ERROR_HEADER
=
Review Comment:
This header is for broker request, not controller.
Seems the recent standard deprecated the `X-` prefix, so I'd suggest naming
it `Pinot-Use-Http-Status-For-Errors`
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1722,7 +1724,7 @@ public static class Realtime {
public enum Status {
IN_PROGRESS, // The segment is still consuming data
COMMITTING, // This state will only be utilised by pauseless ingestion
when the segment has been consumed but
- // is yet to be build and uploaded by the server.
+ // is yet to be build and uploaded by the server.
Review Comment:
(nit) Revert
##########
pinot-spi/pom.xml:
##########
@@ -232,6 +232,10 @@
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
Review Comment:
Move this above (probably above `plexus-classworlds`) and fix the indentation
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -587,21 +587,35 @@ public static HttpRequesterIdentity
makeHttpIdentity(org.glassfish.grizzly.http.
* Generate Response object from the BrokerResponse object with
'X-Pinot-Error-Code' header value
*
* If the query is successful the 'X-Pinot-Error-Code' header value is set
to -1
- * otherwise, the first error code of the broker response exception array
will become the header value
+ * otherwise, the first error code of the broker response exception array
will become the header value.
*
- * @param brokerResponse
+ * By default, returns HTTP 200 OK even for errors. If the request header
+ * 'X-PINOT-HTTP-RESPONSE-CODE-REPRESENT-ERROR' is set to 'true', returns
appropriate HTTP status
+ * codes based on the error type from QueryErrorCode.getHttpResponseStatus().
+ *
+ * @param brokerResponse The broker response containing query results or
errors
+ * @param httpHeaders The HTTP headers from the request
* @return Response
* @throws Exception
*/
@VisibleForTesting
- public static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ public static Response getPinotQueryResponse(BrokerResponse brokerResponse,
HttpHeaders httpHeaders)
throws Exception {
int queryErrorCodeHeaderValue = -1; // default value of the header.
+ Response.Status httpStatus = Response.Status.OK;
+
List<QueryProcessingException> exceptions = brokerResponse.getExceptions();
if (!exceptions.isEmpty()) {
// set the header value as first exception error code value.
queryErrorCodeHeaderValue = exceptions.get(0).getErrorCode();
+ // Check if the client wants actual HTTP error codes instead of 200 OK
+ if ("true".equalsIgnoreCase(httpHeaders.getHeaderString(
Review Comment:
(minor) Use `Boolean.parseBoolean()`
--
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]
