Hi @gaodayue, sorry it took a while to get you feedback on this proposal. I think it looks great. I have some small suggestions:
- `sqlQueryId` and `sqlQuery/time` look better to me than `sqlId` and `sql/time`. It emphasizes that they are, in fact, queries (just SQL instead of native). I do agree with you that either one of these makes more sense than `query/sql/time`. - I think it's better to add a method to RequestLogger that accepts a SQL query, instead of creating a new SqlRequestLogger. I think, on balance, it will be nicer for people to get all their logs in one place rather than having to worry about configuring two different things. - Maybe this is a good opportunity to fix #4826 as well. My thoughts on that are in https://github.com/apache/incubator-druid/issues/4826#issuecomment-330560830. - The `sqlQueryId` sounds like a natural choice for implementing a SQL query cancelation API in the future - do you agree? [ Full content available at: https://github.com/apache/incubator-druid/issues/6301 ] This message was relayed via gitbox.apache.org for [email protected]
