New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split out queued phase from QueryManager #12176

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@dain
Copy link
Contributor

dain commented Jan 3, 2019

Move queued phase of query from QueryManager to a new dispatcher service. This
change is in preparation for adding a optional new server that moves the queue
phase to a separate process.

dain added some commits Oct 18, 2018

Remove system startup minimum worker requirement
The normal minimum worker requirement applied to all queries is sufficient to cover this case.

@dain dain requested a review from raghavsethi Jan 3, 2019

@raghavsethi
Copy link
Contributor

raghavsethi left a comment

Remove system startup minimum worker requirement looks good.

@raghavsethi
Copy link
Contributor

raghavsethi left a comment

Add DISPATCHING query states LGTM.

private void beginDispatching(long now)
{
beginWaitingForResources(now);
resourceWaitingTime.compareAndSet(null, nanosSince(beginResourceWaitingNanos, now));

This comment has been minimized.

@raghavsethi

raghavsethi Jan 4, 2019

Contributor

I'm assuming resource waiting time is deliberately 0 for this commit.

@findepi

This comment has been minimized.

Copy link
Contributor

findepi commented Jan 4, 2019

Remove system startup minimum worker requirement

is this removing a functionality or moving it elsewhere?

@raghavsethi
Copy link
Contributor

raghavsethi left a comment

Still working on Split out queued phase from QueryManager, got as far as InternalResourceGroup

{
this.httpUri = requireNonNull(httpUri, "httpUri is null");
this.httpsUri = requireNonNull(httpsUri, "httpsUri is null");
checkArgument(httpUri.isPresent() || httpsUri.isPresent(), "Coordinator must have a http or https port");

This comment has been minimized.

@raghavsethi

raghavsethi Jan 5, 2019

Contributor

HTTP or HTTPS

checkArgument(httpUri.isPresent() || httpsUri.isPresent(), "Coordinator must have a http or https port");
}

public URI getUri(String desiredScheme)

This comment has been minimized.

@raghavsethi

raghavsethi Jan 5, 2019

Contributor

Style nit: Maybe preferredScheme, it's 'weaker' than desired :)

import static java.util.Objects.requireNonNull;
import static java.util.concurrent.Executors.newScheduledThreadPool;

public class DispatcherQueryManager

This comment has been minimized.

@raghavsethi

raghavsethi Jan 5, 2019

Contributor

Does this perform the same function as the old QueryManager? If not, maybe call this DispatchManager to delineate it from the other one more clearly.

sessionContext.getResourceEstimates(),
queryType));

// apply system defaults for resource group

This comment has been minimized.

@raghavsethi

raghavsethi Jan 5, 2019

Contributor

// apply system defaults for resource group and/or type
or
// apply system defaults

onQueryCreated(dispatchQuery);
dispatchQuery.addStateChangeListener(newState -> {
if (newState == QueryState.FAILED) {
onQueryFailure(dispatchQuery);

This comment has been minimized.

@raghavsethi

raghavsethi Jan 5, 2019

Contributor

Nit: I dislike this handler naming style, would prefer handleQueryFailure(dispatchQuery)

@@ -114,6 +117,38 @@ else if (info.getErrorCode().getCode() == USER_CANCELED.toErrorCode().getCode())
}
}

public void queryFailedWhileQueuedEvent(Duration queuedTime, Optional<ErrorCode> errorCode)

This comment has been minimized.

@raghavsethi

raghavsethi Jan 5, 2019

Contributor

This method name is inconsistent with other methods in this class, rename to queryFailedWhileQueued or queuedQueryFailed

stats.queryQueued();
}

private void onQueryFailure(DispatchQuery dispatchQuery)

This comment has been minimized.

@raghavsethi

raghavsethi Jan 5, 2019

Contributor

Same nit

}
}

public ListenableFuture<?> waitForDispatched(QueryId queryId)

This comment has been minimized.

@raghavsethi

raghavsethi Jan 5, 2019

Contributor

Cmd-F does not show any callsites for this function? The reason I looked is because it seems potentially weird to record heartbeat as a side effect, and that made me wonder when we call this.

@raghavsethi
Copy link
Contributor

raghavsethi left a comment

Some minor issues and test failures. I'm guessing product tests function as sufficient integration testing?

@@ -45,11 +46,14 @@
@Path("/v1/query")
public class QueryResource
{
// todo There should be a combined interface for this

This comment has been minimized.

@raghavsethi

raghavsethi Jan 8, 2019

Contributor

Minor nit: // TODO:

@@ -45,11 +46,14 @@
@Path("/v1/query")
public class QueryResource
{
// todo There should be a combined interface for this
private final DispatcherQueryManager dispatcherQueryManager;
private final QueryManager queryManager;

This comment has been minimized.

@raghavsethi

raghavsethi Jan 8, 2019

Contributor

This is kinda confusing - can we rename QueryManager to be more specific?

return query;
}

// this is he first time the query has been accessed on this coordinator

This comment has been minimized.

@raghavsethi

raghavsethi Jan 8, 2019

Contributor

// this is the

Session session;
try {
if (!queryManager.isQuerySlugValid(queryId, slug)) {
throw badRequest(NOT_FOUND, "Query not found");

This comment has been minimized.

@raghavsethi

raghavsethi Jan 8, 2019

Contributor

It makes sense to 404 from a security perspective, but I'm wondering whether we can make this case easier to spot and debug. I think we also want to write a log line when this happens.

@@ -559,8 +499,9 @@ private synchronized URI createNextResultsUri(String scheme, UriInfo uriInfo)
{
return uriInfo.getBaseUriBuilder()
.scheme(scheme)
.replacePath("/v1/statement")
.replacePath("/v1/statement/executing")

This comment has been minimized.

@raghavsethi

raghavsethi Jan 8, 2019

Contributor

I'm assuming you've talked to @martint or @electrum offline about the specific API change. I know we had a general discussion previously.

assertEquals(resourceGroup.get(), createResourceGroupId("global", "user-user", "reject-all-queries"));
DispatcherQueryManager dispatcherQueryManager = queryRunner.getCoordinator().getDispatcherQueryManager();
BasicQueryInfo basicQueryInfo = dispatcherQueryManager.getQueryInfo(secondQuery);
assertEquals(basicQueryInfo.getErrorCode(), QUERY_QUEUE_FULL.toErrorCode());

This comment has been minimized.

@raghavsethi

raghavsethi Jan 8, 2019

Contributor

Hmm this feels like a different test. Is there a reason why we won't also check the resource group?

QueryManager queryManager = queryRunner.getCoordinator().getQueryManager();
QueryId queryId = queryManager.createQueryId();
queryManager.createQuery(
DispatcherQueryManager dispatcherQueryManager = queryRunner.getCoordinator().getDispatcherQueryManager();

This comment has been minimized.

@raghavsethi

raghavsethi Jan 8, 2019

Contributor

You're doing this again on L126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment