yashmayya commented on code in PR #14951:
URL: https://github.com/apache/pinot/pull/14951#discussion_r1937064122
##########
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNative.java:
##########
@@ -110,6 +110,10 @@ public BrokerResponseNative(ProcessingException exception)
{
_exceptions.add(new QueryProcessingException(exception.getErrorCode(),
exception.getMessage()));
}
+ public BrokerResponseNative(int errorCode, String errorMessage) {
+ _exceptions.add(new QueryProcessingException(errorCode, errorMessage));
Review Comment:
Maybe we should rename the `QueryProcessingException` class to distinguish
it better from the new `QException` class? Also that class is not really an
`Exception` anyway.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -398,18 +414,13 @@ private String getQueryURL(String protocol, String
hostName, int port) {
return String.format("%s://%s:%d/query/sql", protocol, hostName, port);
}
- public String sendPostRaw(String urlStr, String requestStr, Map<String,
String> headers) {
+ public void sendPostRaw(String urlStr, String requestStr, Map<String,
String> headers, OutputStream outputStream) {
HttpURLConnection conn = null;
try {
- /*if (LOG.isInfoEnabled()){
+ if (LOGGER.isInfoEnabled()) {
LOGGER.info("Sending a post request to the server - " + urlStr);
Review Comment:
This should be broker instead of server right?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/exception/BadQueryRequestException.java:
##########
@@ -18,16 +18,28 @@
*/
package org.apache.pinot.spi.exception;
-public class BadQueryRequestException extends RuntimeException {
+public class BadQueryRequestException extends QException {
public BadQueryRequestException(String message) {
- super(message);
+ super(SQL_RUNTIME_ERROR_CODE, message);
Review Comment:
This class seems to be used for user errors - why do we want to use
`SQL_RUNTIME_ERROR_CODE` for that?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -104,59 +104,61 @@ public class PinotQueryResource {
@POST
@Path("sql")
@ManualAuthorization // performed by broker
- public String handlePostSql(String requestJsonStr, @Context HttpHeaders
httpHeaders) {
+ public StreamingOutput handlePostSql(String requestJsonStr, @Context
HttpHeaders httpHeaders) {
+ JsonNode requestJson;
try {
- JsonNode requestJson = JsonUtils.stringToJsonNode(requestJsonStr);
- if (!requestJson.has("sql")) {
- return
constructQueryExceptionResponse(QueryException.getException(QueryException.JSON_PARSING_ERROR,
- "JSON Payload is missing the query string field 'sql'"));
- }
- String sqlQuery = requestJson.get("sql").asText();
- String traceEnabled = "false";
- if (requestJson.has("trace")) {
- traceEnabled = requestJson.get("trace").toString();
- }
- String queryOptions = null;
- if (requestJson.has("queryOptions")) {
- queryOptions = requestJson.get("queryOptions").asText();
- }
- LOGGER.debug("Trace: {}, Running query: {}", traceEnabled, sqlQuery);
- return executeSqlQuery(httpHeaders, sqlQuery, traceEnabled,
queryOptions, "/sql");
- } catch (ProcessingException pe) {
- LOGGER.error("Caught exception while processing post request {}",
pe.getMessage());
- return constructQueryExceptionResponse(pe);
- } catch (WebApplicationException wae) {
- LOGGER.error("Caught exception while processing post request", wae);
- throw wae;
+ requestJson = JsonUtils.stringToJsonNode(requestJsonStr);
} catch (Exception e) {
- LOGGER.error("Caught exception while processing post request", e);
- return
constructQueryExceptionResponse(QueryException.getException(QueryException.INTERNAL_ERROR,
e));
+ return
constructQueryExceptionResponse(QueryException.JSON_PARSING_ERROR_CODE,
e.getMessage());
+ }
+ if (!requestJson.has("sql")) {
+ return
constructQueryExceptionResponse(QueryException.getException(QueryException.JSON_PARSING_ERROR,
+ "JSON Payload is missing the query string field 'sql'"));
+ }
+ String sqlQuery = requestJson.get("sql").asText();
+ String traceEnabled = "false";
+ if (requestJson.has("trace")) {
+ traceEnabled = requestJson.get("trace").toString();
+ }
+ String queryOptions = null;
+ if (requestJson.has("queryOptions")) {
+ queryOptions = requestJson.get("queryOptions").asText();
}
+
+ return executeSqlQueryCatching(httpHeaders, sqlQuery, traceEnabled,
queryOptions);
}
@GET
@Path("sql")
@ManualAuthorization
- public String handleGetSql(@QueryParam("sql") String sqlQuery,
@QueryParam("trace") String traceEnabled,
+ public StreamingOutput handleGetSql(@QueryParam("sql") String sqlQuery,
@QueryParam("trace") String traceEnabled,
@QueryParam("queryOptions") String queryOptions, @Context HttpHeaders
httpHeaders) {
+ return executeSqlQueryCatching(httpHeaders, sqlQuery, traceEnabled,
queryOptions);
+ }
+
+ private StreamingOutput executeSqlQueryCatching(HttpHeaders httpHeaders,
String sqlQuery, String traceEnabled,
+ String queryOptions) {
try {
- LOGGER.debug("Trace: {}, Running query: {}", traceEnabled, sqlQuery);
- return executeSqlQuery(httpHeaders, sqlQuery, traceEnabled,
queryOptions, "/sql");
+ return executeSqlQuery(httpHeaders, sqlQuery, traceEnabled,
queryOptions);
+ } catch (QException e) {
+ LOGGER.error("Caught query exception while processing post request", e);
Review Comment:
Why can't this occur for `GET` requests? And similarly, why can't the
subsequent exceptions occur for `POST` requests?
##########
pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java:
##########
@@ -23,6 +23,7 @@
import java.util.regex.Pattern;
import org.apache.commons.lang3.StringUtils;
import org.apache.pinot.common.response.ProcessingException;
+import org.apache.pinot.spi.exception.QException;
Review Comment:
While modifying this class, I think we can rename it as well to avoid the
confusion with new `QException` (I see that there's already a TODO for that
here)?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -210,51 +217,62 @@ private String getMultiStageQueryResponse(String query,
String queryOptions, Htt
queryOptionsMap.putAll(RequestUtils.getOptionsFromString(queryOptions));
}
String database =
DatabaseUtils.extractDatabaseFromQueryRequest(queryOptionsMap, httpHeaders);
+ List<String> tableNames = getTableNames(query, database);
+ List<String> instanceIds = getInstanceIds(query, tableNames, database);
+ String instanceId = selectRandomInstanceId(instanceIds);
+ return sendRequestToBroker(query, instanceId, traceEnabled, queryOptions,
httpHeaders);
+ }
+
+ private List<String> getTableNames(String query, String database) {
QueryEnvironment queryEnvironment =
new QueryEnvironment(database,
_pinotHelixResourceManager.getTableCache(), null);
List<String> tableNames;
try {
tableNames = queryEnvironment.getTableNamesForQuery(query);
+ } catch (QException e) {
+ if (e.getErrorCode() != QueryException.UNKNOWN_ERROR_CODE) {
+ throw e;
+ } else {
+ throw new QException(QException.SQL_PARSING_ERROR_CODE, e);
+ }
} catch (Exception e) {
- return QueryException.getException(QueryException.SQL_PARSING_ERROR,
- new Exception("Unable to find table for this query", e)).toString();
+ throw new QException(QException.SQL_PARSING_ERROR_CODE, e);
Review Comment:
This part is a little confusing - why are we treating unknown error as
parsing error but not handling other types of `QException` here?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -398,18 +414,13 @@ private String getQueryURL(String protocol, String
hostName, int port) {
return String.format("%s://%s:%d/query/sql", protocol, hostName, port);
}
- public String sendPostRaw(String urlStr, String requestStr, Map<String,
String> headers) {
+ public void sendPostRaw(String urlStr, String requestStr, Map<String,
String> headers, OutputStream outputStream) {
HttpURLConnection conn = null;
try {
- /*if (LOG.isInfoEnabled()){
+ if (LOGGER.isInfoEnabled()) {
Review Comment:
nit: is this really needed?
--
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]
