Hi @gianm , thanks for your 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.
Sure, sqlQueryId and sqlQuery/time express more accurate meaning. I'll also change `X-Druid-SQL-Id` to `X-Druid-SQL-Query-Id`. > 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. I think adding a separate `SqlRequestLogger` is more flexible than reusing `RequestLogger`, for example - It allows users to enable sql logging while disabling native query logging, which can be useful in cases where only SQL is used. - It allows users to choose different implementations for SQL and native query logging. For instance, users may want to log sql request to both kafka and files, while only keep native query logs in local files for diagnostic purpose. > Maybe this is a good opportunity to fix #4826 as well. My thoughts on that > are in #4826 (comment). Yeah, but I think it's better to address that issue in a separate PR for backport purpose. Let me finish this first :) > The sqlQueryId sounds like a natural choice for implementing a SQL query > cancelation API in the future - do you agree? Totally agree. [ Full content available at: https://github.com/apache/incubator-druid/issues/6301 ] This message was relayed via gitbox.apache.org for [email protected]
