jihoonson edited a comment on pull request #10464: URL: https://github.com/apache/druid/pull/10464#issuecomment-721296649
Hi @a2l007 and @capistrant, thanks for the nice PR and review! We usually treat silence as acceptance, so thanks for merging the PR too. However, for this particular PR, I think it needs to get "Design Review" which needs 3 +1s from committers (including the author himself) because it changes some user-facing behavior (https://github.com/apache/druid/blob/master/dev/committer-instructions.md). We have this policy because it's hard to undo once the changed user-facing behavior appears in some release. When we do design review, we usually have one committer reviewing the whole PR line by line, so that others can focus on only the design part. From this aspect, I quickly reviewed only the design part. I have 3 comments so far. - The purpose of the PR sounds good to me because the timeout error reporting was inconsistent before. However this change is incompatible which means it can break user applications working based on the previous behavior. Someone can argue that no one could do it because the previous behavior was inconsistent. IMO, this change looks worth since the previous timeout error handling was easy to break, but should be called out in the release notes. - Looking at the code, [the status code is defined as 504](https://github.com/apache/druid/pull/10464/files#diff-4bc40182ca7938163c8f2b71355cb5849920526319bc56f83dd55e542f3a4323R42) which means gateway timeout. I'm wondering 408 request timeout is more appropriate in this case. - All query errors should be documented. Check out https://druid.apache.org/docs/latest/querying/querying.html#query-errors. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
