paul-rogers commented on code in PR #12845: URL: https://github.com/apache/druid/pull/12845#discussion_r935988484
########## sql/src/main/java/org/apache/druid/sql/AbstractStatement.java: ########## @@ -0,0 +1,203 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.sql; + +import org.apache.calcite.sql.parser.SqlParseException; +import org.apache.calcite.tools.ValidationException; +import org.apache.druid.java.util.common.logger.Logger; +import org.apache.druid.query.QueryContext; +import org.apache.druid.query.QueryContexts; +import org.apache.druid.server.security.Access; +import org.apache.druid.server.security.AuthorizationUtils; +import org.apache.druid.server.security.ForbiddenException; +import org.apache.druid.server.security.ResourceAction; +import org.apache.druid.sql.calcite.planner.DruidPlanner; +import org.apache.druid.sql.calcite.planner.PlannerContext; +import org.apache.druid.sql.calcite.planner.PlannerResult; + +import java.io.Closeable; +import java.util.Set; +import java.util.UUID; +import java.util.function.Function; + +/** + * Represents a SQL statement either for preparation or execution. Review Comment: Great question. By "beforehand", do you mean "before we close the statement"? The example was "an exception while calling the public methods of the class." Let's start by defining the protocol for the new code. An instance is created with the query info. Then, the query is prepared or run, which can fail. If the calls fail, we pass the exception to the reporter class. Regardless of success or failure, the caller closes the statement. On close, the reporter will emit either the success or failure case. The only way to avoid reporting is to call `closeQuietly()`, as in tests. Judging by the existing code, it seems we always emit logs and and metrics at the conclusion of the lifecycle: there is a pile of code in `SqlResource` to do that, as well as code in the JDBC classes. The JDBC tests actually verify that the proper log info is produced. In that context, we emit logs and metrics for both successful and failed queries. There was some ambiguity about failures due to a security violation: that info was _not_ logged previously, but a comment suggested this was an undesired limitation rather than a feature. This PR adds logging even for security failures. To recap, the result is that, if `close()` is called, logs and metrics are emitted for both successful and failed queries. For tests, we call `closeQuietly()` since there is no need to emit this information. -- 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]
