[GitHub] [pinot] jasperjiaguo commented on pull request #10683: Refine the return error code of query killing

["jasperjiaguo (via GitHub)" <gi...@apache.org>]

jasperjiaguo commented on PR #10683:
URL: https://github.com/apache/pinot/pull/10683#issuecomment-1530101191

   > > @walterddr agreed, I think we should consider the error codes holistically sometime, it is indeed a bit misleading right now. This PR should not affect the future refactoring right? I don't think we depend on hard coded number here?
   > 
   > hmm. i think we should agree upon the error code convension we wanted to use first. until then we should make this PR's query killing behavior return 200 with attach the exception on the JSON return payload just like the rest of the error query response. since the return code is used by the REST client it would be particularly difficult for backward-compatibility modifications going forward (say tomorrow we decided to return a 4xx instead of 205) some browser will consider non-2xx as exception instead of an acceptable return which will cause confusion IMO
   
   Hey Rong, I completely understand your concern of backward-compatibility. We definitely want a consistency with the standard http error code for our query return codes going forward. However, there is still a possibility we do this in an incremental way for this query cancellation code, the reasons are
   
   1.  Currently when a query is killed/cancelled on a broker it already returns 205 instead of 200, the following is from our log:
   ```
   Got 1 errors for request: *****, 
   Errors:
   Error code: 205, Error message: org.apache.pinot.spi.exception.EarlyTerminationException:
   ```
   IMO it's better for us to converge the code here and let server return the same for the interruption behavior
   
   2. Currently we return all execution errors as 200 therefore some web browsers would take all execution failures as some kind of success, in this case, would changing from 200 -> 205 make it worse? Do you have deployments that depend on the hard-coded 200 for interruption errors?
   
   This problem is currently impacting our deployment. PTAL cc @walterddr @siddharthteotia 


-- 
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