You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "abhioncbr (via GitHub)" <gi...@apache.org> on 2024/01/30 02:53:58 UTC
[PR] Added pinot-error-code header in query response [pinot]
abhioncbr opened a new pull request, #12338:
URL: https://github.com/apache/pinot/pull/12338
As per the [issue](https://github.com/apache/pinot/issues/12190), added pinot-error-code header in query response
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1480173365
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
think future use cases. now it is used to differentiate whether a query has failed. (b/c the HTTP return code is 200); if in the future we also want to know if the failure is retryable how do we encode that info.
the header now is only used as "key-only" it doesn't actually parse the value IIUC <-- @ankitsultana
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1501737023
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
Review Comment:
It's updated with the changes and UTs. Can you please have a look? Thanks
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1498420180
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
> or do we want to use a different header
In the long term if we also want to report whether an error is retryable, or a client error, then we would have to add a new header.
Ideally the error-codes in QueryException should have followed some convention (like all 4xx are user errors, and 5xx are internal errors, etc.).
But given the business logic is coded into Pinot itself, I don't think we have any choice other than converting the error-code into the appropriate flag in Pinot and returning it in a dedicated header.
e.g. for marking an error as a client-error, we could potentially add another header like `X-Pinot-Client-Error: true`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1498635402
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
Review Comment:
Sure. Let me add/update the UTs as well. Thanks
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1473591396
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
it is a bit weird to response "OK" then with an "ERROR code" IMO
CC @ankitsultana to take a look
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1475351199
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
got it. will wait for a bit of feedback. was wondering if we should take similar naming with QueryException (which already have a number-based error 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1474714391
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
Yeah the issue is that the broker API returns exceptions in the JSON in most scenarios, with the HTTP Status Code set to 200. We can't change that behavior without making it backwards incompatible, hence the proposal was to add an additional response header.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1498619033
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
Review Comment:
@abhioncbr : can you rename `headerValue` to something more precise? In the context of this PR this makes sense, but generally we may add more headers here later.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
Review Comment:
Can you add test summary in the description?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #12338:
URL: https://github.com/apache/pinot/pull/12338
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1480173365
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
think future use cases. now it is used to differentiate whether a query has failed. (b/c the HTTP return code is 200); if in the future we also want to know if the failure is retryable how do we encode that info; do we want the client to understand all the values in the QueryException numbers, or do we want to use a different header.
the header now is only used as "key-only" it doesn't actually parse the value IIUC <-- @ankitsultana
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1479207895
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
@walterddr can you please explain more about similar naming with QueryException? Thanks
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1498561873
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
I agree. In the future, we can extend this header approach based on the use-case requirements.
Also, as @ankitsultana mentioned, we should have a convention around error codes, but we need to figure out how end users use it and how we can introduce better error codes with backward compatibility.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12338:
URL: https://github.com/apache/pinot/pull/12338#issuecomment-1916010575
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12338?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `10 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`e1d20d7`)](https://app.codecov.io/gh/apache/pinot/commit/e1d20d75d5f641b8a36d52271716c811efcb6484?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.62% compared to head [(`6ff9595`)](https://app.codecov.io/gh/apache/pinot/pull/12338?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 34.76%.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12338?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pinot/broker/api/resources/PinotClientRequest.java](https://app.codecov.io/gh/apache/pinot/pull/12338?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | 0.00% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12338?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12338 +/- ##
=============================================
- Coverage 61.62% 34.76% -26.86%
=============================================
Files 2421 2345 -76
Lines 131872 128130 -3742
Branches 20346 19789 -557
=============================================
- Hits 81260 44548 -36712
- Misses 44644 80395 +35751
+ Partials 5968 3187 -2781
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (?)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.76% <0.00%> (+7.03%)` | :arrow_up: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.76% <0.00%> (+7.04%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.76% <0.00%> (-26.86%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.58% <ø> (-15.04%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.58% <ø> (-0.13%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12338/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12338?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1480163071
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
was referring to the handling of these numbers:
https://github.com/apache/pinot/blob/9314c116adf0330a3c4b38d722f53a857a618add/pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java#L49
for example at the bottom there's client error. was wondering if we want to differentiate with query error code header between client/broker/server error
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
was referring to the handling of these numbers:
https://github.com/apache/pinot/blob/9314c116adf0330a3c4b38d722f53a857a618add/pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java#L49
for example at the bottom there's client error. was wondering if we want to differentiate with query error code header key between client/broker/server error
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added pinot-error-code header in query response [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #12338:
URL: https://github.com/apache/pinot/pull/12338#discussion_r1480163071
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -348,4 +349,25 @@ private static HttpRequesterIdentity makeHttpIdentity(org.glassfish.grizzly.http
return identity;
}
+
+ /**
+ * Generate Response object from the BrokerResponse object with 'X-Pinot-Error-Code' header value
+ * @param brokerResponse
+ * @return Response
+ * @throws Exception
+ */
+ private static Response getPinotQueryResponse(BrokerResponse brokerResponse)
+ throws Exception {
+ int headerValue = -1; // default value of the header.
+
+ if (brokerResponse.getExceptionsSize() != 0) {
+ // set the header value as first exception error code value.
+ headerValue = brokerResponse.getProcessingExceptions().get(0).getErrorCode();
+ }
+
+ // returning the Response with OK status and header value.
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, headerValue)
Review Comment:
https://github.com/apache/pinot/blob/9314c116adf0330a3c4b38d722f53a857a618add/pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java#L49
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org