clintropolis commented on code in PR #14004: URL: https://github.com/apache/druid/pull/14004#discussion_r1231824195
########## processing/src/main/java/org/apache/druid/error/README.md: ########## @@ -0,0 +1,404 @@ +<!-- + ~ 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. + --> + +WARNING WARNING +TODO: this README has not been adjusted to align with the current code Review Comment: is this still true? (maybe it should be refreshed to align with the current code before we merge?) also any possible way to add something like a 'tl;dr' to summarize the important parts? 😛 ########## processing/src/main/java/org/apache/druid/error/ErrorResponse.java: ########## @@ -0,0 +1,214 @@ +/* + * 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.error; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonValue; +import org.apache.druid.query.QueryException; + +import javax.annotation.Nullable; +import java.util.LinkedHashMap; +import java.util.Map; + +/** + * A Response Object that represents an error to be returned over the wire. This object carries legacy bits to + * deal with compatibility issues of converging the error responses from {@link QueryException} + * with the intended going-forward error responses from {@link DruidException} + * <p> + * The intent is that eventually {@link QueryException} is completely subsumed by + * {@link DruidException} in which case the legacy bits of this class can hopefully also be removed. + * <p> + * The intended long-term schema of output is an object that looks like + * <p> + * { + * "errorCode": `a code string`, + * "persona": USER | ADMIN | OPERATOR | DEVELOPER + * "category": DEFENSIVE | INVALID_INPUT | UNAUTHORIZED | CAPACITY_EXCEEDED | CANCELED | RUNTIME_FAILURE | TIMEOUT | UNSUPPORTED | UNCATEGORIZED + * "errorMessage": `a message for the intended audience` + * "context": `a map of extra context values that might be helpful` + * } + * <p> + * In the interim, there are extra fields that also end up included so that the wire-schema can also be interpretted + * and handled by clients that are built assuming they are looking at QueryExceptions. These extra fields are + * <p> + * { + * "error": `an error code from QueryException` | "druidException" + * "errorClass": `the error class, as used by QueryException` + * "host": `the host that the exception occurred on, as used by QueryException` + * } + * <p> + * These 3 top-level fields are deprecated and will eventually disappear from API responses. The values can, instead, + * be pulled from the context object of an "legacyQueryException" errorCode object. The field names in the context + * object map as follows + * * "error" -> "legacyErrorCode" + * * "errorClass" -> "errorClass" + * * "host" -> "host" + */ +public class ErrorResponse +{ + @JsonCreator + public static ErrorResponse fromMap(Map<String, Object> map) + { + final DruidException.Failure failure; + + final Object legacyErrorType = map.get("error"); + if (!"druidException".equals(legacyErrorType)) { + // The non "druidException" errorCode field means that we are deserializing a legacy QueryException object rather + // than deserializing a DruidException. So, we make a QueryException, map it to a DruidException and build + // our response from that DruidException. This allows all code after us to only consider DruidException + // and helps aid the removal of QueryException. + failure = new QueryExceptionCompat( + new QueryException( + nullOrString(map.get("error")), + nullOrString(map.get("errorMessage")), + nullOrString(map.get("errorClass")), + nullOrString(map.get("host")) + ) + ); + } else { + failure = new DruidException.Failure(stringOrFailure(map, "errorCode")) + { + @Override + protected DruidException makeException(DruidException.DruidExceptionBuilder bob) + { + final DruidException retVal = bob.forPersona(DruidException.Persona.valueOf(stringOrFailure(map, "persona"))) + .ofCategory(DruidException.Category.valueOf(stringOrFailure( + map, + "category" + ))) + .build(stringOrFailure(map, "errorMessage")); + + final Object context = map.get("context"); + if (context instanceof Map) { + //noinspection unchecked + retVal.withContext((Map<String, String>) context); + } + + return retVal; + } + }; + } + return new ErrorResponse(DruidException.fromFailure(new DeserializedFailure(failure))); + } + + private final DruidException underlyingException; + + public ErrorResponse(DruidException underlyingException) + { + this.underlyingException = underlyingException; + } + + @JsonValue + public Map<String, Object> getAsMap() + { + final LinkedHashMap<String, Object> retVal = new LinkedHashMap<>(); + + // This if statement is a compatibility layer to help bridge the time while we are introducing the DruidException. + // In a future release, QueryException should be completely eliminated, at which point we should also be + // able to eliminate this compatibility layer. + if (QueryExceptionCompat.ERROR_CODE.equals(underlyingException.getErrorCode())) { + retVal.put("error", underlyingException.getContextValue("legacyErrorCode")); + retVal.put("errorClass", underlyingException.getContextValue("errorClass")); + retVal.put("host", underlyingException.getContextValue("host")); + } else { + retVal.put("error", "druidException"); + } + + retVal.put("errorCode", underlyingException.getErrorCode()); + retVal.put("persona", underlyingException.getTargetPersona().toString()); + retVal.put("category", underlyingException.getCategory().toString()); + retVal.put("errorMessage", underlyingException.getMessage()); + retVal.put("context", underlyingException.getContext()); + + return retVal; + } + + public DruidException getUnderlyingException() + { + return underlyingException; + } + + @Nullable + private static String nullOrString(Object o) Review Comment: there is a similar existing method, `Evals.asString` though its primarily used for processing data for expressions and query time stuff, so i don't feel super strongly about using it here or not ########## processing/src/main/java/org/apache/druid/error/InvalidSqlInput.java: ########## @@ -0,0 +1,50 @@ +/* + * 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.error; + +public class InvalidSqlInput extends InvalidInput Review Comment: heh, that isn't really possible without also merging druid-server into processing, or alternatively splitting druid-sql operators and stuff into processing and putting the schema and higher level stuff that builds on broker stuff in druid-server (which might also be nice, but would make testing slightly more painful maybe?) ########## server/src/test/java/org/apache/druid/server/QueryResourceTest.java: ########## @@ -296,6 +302,79 @@ public void testGoodQueryWithQueryConfigOverrideDefault() throws IOException ); } + @Test + public void testGoodQueryThrowsDruidExceptionFromLifecycleExecute() throws IOException + { + String overrideConfigKey = "priority"; + String overrideConfigValue = "678"; + DefaultQueryConfig overrideConfig = new DefaultQueryConfig(ImmutableMap.of(overrideConfigKey, overrideConfigValue)); + queryResource = new QueryResource( + new QueryLifecycleFactory( + WAREHOUSE, + new QuerySegmentWalker() + { + @Override + public <T> QueryRunner<T> getQueryRunnerForIntervals( + Query<T> query, + Iterable<Interval> intervals + ) + { + throw DruidException.forPersona(DruidException.Persona.OPERATOR) + .ofCategory(DruidException.Category.RUNTIME_FAILURE) + .build("failing for coverage!"); + } + + @Override + public <T> QueryRunner<T> getQueryRunnerForSegments( + Query<T> query, + Iterable<SegmentDescriptor> specs + ) + { + throw new UnsupportedOperationException(); + } + }, + new DefaultGenericQueryMetricsFactory(), + new NoopServiceEmitter(), + testRequestLogger, + new AuthConfig(), + AuthTestUtils.TEST_AUTHORIZER_MAPPER, + Suppliers.ofInstance(overrideConfig) + ), + jsonMapper, + smileMapper, + queryScheduler, + new AuthConfig(), + null, + ResponseContextConfig.newConfig(true), + DRUID_NODE + ); + + expectPermissiveHappyPathAuth(); + + final Response response = expectSynchronousRequestFlow(SIMPLE_TIMESERIES_QUERY); + Assert.assertEquals(Status.INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus()); + + final ErrorResponse entity = (ErrorResponse) response.getEntity(); Review Comment: ah i guess this is the serde test i was looking for? ########## processing/src/main/java/org/apache/druid/error/DruidException.java: ########## @@ -0,0 +1,389 @@ +/* + * 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.error; + +import com.fasterxml.jackson.annotation.JsonValue; +import com.google.common.base.Preconditions; +import org.apache.druid.java.util.common.StringUtils; + +import javax.annotation.concurrent.NotThreadSafe; +import java.util.LinkedHashMap; +import java.util.Map; + +/** + * Represents an error condition exposed to the user and/or operator of Druid. Given that a DruidException is intended + * to be delivered to the end user, it should generally never be caught. DruidExceptions are generated at terminal + * points where the operation that was happening cannot make forward progress. As such, the only reason to catch a + * DruidException is if the code has some extra context that it wants to add to the message of the DruidException using + * {@link #prependAndBuild(String, Object...)}. If code wants to catch and handle an exception instead, it should not + * be using the DruidException. Review Comment: i think there is still a use case for them maybe for low level exceptions that don't have the full picture because they are missing some context to provide a meaningful exception at the surface. In these cases I expect the code area which does have the context will catch IAE/ISE/UOE etc and then extract the details and decorate with additional context such as column and table name, etc. That seems preferable to me than having to push all of this information down all of the way to the bottom, particularly in more 'hot' areas of the code since passing arguments isn't completely free. Though, I suspect some of these cases are probably better to use more specific exceptions dedicated to a purpose, _or_ I guess we could still throw `DruidException` but catch it at a higher level to decorate with the additional context, so I think we'll just have to see how it works out in practice. ########## sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java: ########## @@ -307,4 +317,186 @@ public PlannerHook hook() return hook; } } + + public static DruidException translateException(Exception e) + { + try { + throw e; + } Review Comment: any reason not to just make all of these `instanceof` checks, especially since some of the catch still have `instanceof`? ########## processing/src/test/java/org/apache/druid/error/ErrorResponseTest.java: ########## @@ -0,0 +1,110 @@ +/* + * 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.error; + +import com.google.common.collect.ImmutableMap; +import org.apache.druid.matchers.DruidMatchers; +import org.apache.druid.query.QueryTimeoutException; +import org.hamcrest.Matcher; +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; +import org.junit.Test; + +import java.util.Map; + +public class ErrorResponseTest Review Comment: since ErrorResponse is an 'over the wire' thingo, should serde tests be here? (or did i miss them somewhere?) -- 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]
