clintropolis commented on a change in pull request #11643:
URL: https://github.com/apache/druid/pull/11643#discussion_r701387477
##########
File path: sql/src/main/java/org/apache/druid/sql/SqlLifecycle.java
##########
@@ -372,15 +383,38 @@ public void after(boolean isDone, Throwable thrown)
@VisibleForTesting
public ValidationResult runAnalyzeResources(AuthenticationResult
authenticationResult)
{
- synchronized (lock) {
- return validate(authenticationResult);
+ return validate(authenticationResult);
+ }
+
+ public Set<Resource> getAuthorizedResources()
+ {
+ assert validationResult != null;
+ return validationResult.getResources();
+ }
+
+ /**
+ * Cancel all native queries associated to this lifecycle.
+ *
+ * This method is thread-safe.
+ */
+ public void cancel()
+ {
+ canceled = true;
+
+ final CopyOnWriteArrayList<String> nativeQueryIds =
plannerContext.getNativeQueryIds();
+
+ for (String nativeQueryId : nativeQueryIds) {
+ log.debug("canceling native query [%s]", nativeQueryId);
+ queryScheduler.cancelQuery(nativeQueryId);
}
}
- public RelDataType rowType()
+ public Optional<RelDataType> rowType()
{
- synchronized (lock) {
- return plannerResult != null ? plannerResult.rowType() :
prepareResult.getRowType();
+ if (canceled) {
+ return Optional.empty();
Review comment:
What you said about no-op only only seems true for the methods that
transition lifecycle state. The methods which just get stuff, like
`sqlQueryId()` don't seem to become no-ops and return empty when cancelled,
which is why this seemed strange to me.
I guess this method is sort of strange because it actually expects to either
`prepare()` or `plan()` to have been called, otherwise either `planResult` or
`prepareResult` can be null (or it should null check it too). `DruidStatement`
ensures that the statement is in the `PREPARED` state (it has different states
than SqlLifecycle, which is fun) is in the correct state before calling this
method, and `SqlResource` only calls it after it it has called `plan`, so
checking for `null` `prepareResult` isn't strictly necessary, but might be nice
to add.
##########
File path: sql/src/main/java/org/apache/druid/sql/SqlLifecycle.java
##########
@@ -280,22 +277,20 @@ private void checkAccess(Access access)
*/
public PrepareResult prepare() throws RelConversionException
{
- synchronized (lock) {
- if (state != State.AUTHORIZED) {
- throw new ISE("Cannot prepare because current state[%s] is not [%s].",
state, State.AUTHORIZED);
- }
- Preconditions.checkNotNull(plannerContext, "Cannot prepare,
plannerContext is null");
- try (DruidPlanner planner =
plannerFactory.createPlannerWithContext(plannerContext)) {
- this.prepareResult = planner.prepare(sql);
- return prepareResult;
- }
- // we can't collapse catch clauses since SqlPlanningException has
type-sensitive constructors.
- catch (SqlParseException e) {
- throw new SqlPlanningException(e);
- }
- catch (ValidationException e) {
- throw new SqlPlanningException(e);
- }
+ if (state != State.AUTHORIZED) {
Review comment:
since `prepare` doesn't transition state, it might be nice to check for
cancelled here so we don't do silly stuff.
I guess alternatively we could have added `State.CANCELLED`, since then all
of the state transitions would naturally fail because the expected previous
state would not match expectations and `transition` wouldn't have to be a
boolean that callers check. This was seems ok too though, so up to you if you
want to make `CANCELLED` a real state or not.
--
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]
