jihoonson commented on pull request #12026: URL: https://github.com/apache/druid/pull/12026#issuecomment-1055088634
@FrankChen021 my apologies, I forgot to take another look. Please feel free to ping me if I forget again and don't finish my review in time. I took another look. Thank you for adding more tests to satisfy the coverage bot :slightly_smiling_face: The general approach you took in this PR looks good to me. It's much better-structured compared to what we had previously. However, I still have a concern about the user-facing API changes, which I mentioned in https://github.com/apache/druid/pull/12026#discussion_r770186406. I agree that your approach is better than what we have now because it makes the error response format consistent. I think we should apply the same approach to every user-facing API. And I think this should happen at once because it will cause incompatibility in the error response format for some APIs. Suppose you have a client that does something when an API returns an error. For example, it could retry the failed call. If the error response format changes after this PR, you will have to modify your client code to handle the new response format. Even though the new response format is better than the previo us, it will be annoying our users if we introduce such an incompatible change over multiple releases. Since we will probably start preparing a new release sooner or later, we may not be able to have enough time to apply the same technique you used in this PR to every user-facing API. So, how about splitting your PR into two, one that fixes the vulnerability only and another that fixes the error response format? -- 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]
