jihoonson commented on a change in pull request #11643:
URL: https://github.com/apache/druid/pull/11643#discussion_r701492169
##########
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:
I added a new final state, `CANCELLED`.
##########
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.
This is more correct. I should have said state transition becomes no-op. I
updated the comment in the code.
> 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.
I agree this and other methods that I changed to return `Optional` seem
strange. So, instead of that, I added `SqlRowTransformer` to do the necessary
things without exposing stuffs in `SqlLifecycle` to outside. For this
particular method of `rowType`, it is not used in JDBC as `DruidStatement` gets
rowType directly from `PrepareResult` instead of `SqlLifecycle`. I think I
would probably make `plan()` to return `PlannerResult` and do something similar
if `PlannerResult` was as simple as `PrepareResult` and did not have
`resultSupplier`. But since it does have `resultSupplier`, I think it's better
to not expose it to outside but use `SqlRowTransformer`.
--
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]
