anshul98ks123 commented on code in PR #19716:
URL: https://github.com/apache/pinot/pull/19716#discussion_r4336606414
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -410,6 +412,7 @@ public static long getRandomInitialDelayInSeconds() {
public static final int DEFAULT_TABLE_MIN_REPLICAS = 1;
public static final int DEFAULT_JERSEY_ADMIN_PORT = 21000;
public static final boolean DEFAULT_INGEST_FROM_URI_ALLOW_LOCAL_FILE_SYSTEM
= false;
+ public static final boolean DEFAULT_API_ERROR_RESPONSE_INCLUDE_CAUSES = true;
Review Comment:
Fixed in 70d6fa56ab: it is opt-in now. Only `/ingestFromFile` record
failures use `ControllerApplicationException.withCausesInResponse`, and the
config is gone.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/WebApplicationExceptionMapper.java:
##########
@@ -40,7 +69,34 @@ public Response toResponse(Throwable t) {
} else {
status = ((WebApplicationException) t).getResponse().getStatus();
}
- SimpleHttpErrorInfo errorInfo = new SimpleHttpErrorInfo(status,
t.getMessage());
+ SimpleHttpErrorInfo errorInfo = new SimpleHttpErrorInfo(status,
getErrorMessage(t));
return
Response.status(status).entity(errorInfo).type(MediaType.APPLICATION_JSON).build();
}
+
+ @VisibleForTesting
+ @Nullable
+ String getErrorMessage(Throwable t) {
+ if (!shouldIncludeCauses()) {
+ return t.getMessage();
+ }
+ String message = t.getMessage();
+ if (StringUtils.isBlank(message) && !(t instanceof
WebApplicationException) && t.getCause() == null) {
+ String simpleName = t.getClass().getSimpleName();
+ return simpleName.isEmpty() ? t.getClass().getName() : simpleName;
+ }
+ return ExceptionUtils.appendCauses(message, t.getCause(), MAX_CAUSES,
MAX_CAUSE_LENGTH);
Review Comment:
Fixed in 70d6fa56ab: the mapper appends causes only for exceptions created
with `withCausesInResponse`, so raw throwables answer as on master. The worst
case for the first revision was an access-control plugin failing on
`/auth/verify` or `/auth/info` before authentication, with its LDAP or database
host in the cause, reaching an anonymous caller.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/WebApplicationExceptionMapper.java:
##########
@@ -40,7 +69,34 @@ public Response toResponse(Throwable t) {
} else {
status = ((WebApplicationException) t).getResponse().getStatus();
}
- SimpleHttpErrorInfo errorInfo = new SimpleHttpErrorInfo(status,
t.getMessage());
+ SimpleHttpErrorInfo errorInfo = new SimpleHttpErrorInfo(status,
getErrorMessage(t));
return
Response.status(status).entity(errorInfo).type(MediaType.APPLICATION_JSON).build();
}
+
+ @VisibleForTesting
+ @Nullable
+ String getErrorMessage(Throwable t) {
+ if (!shouldIncludeCauses()) {
+ return t.getMessage();
+ }
+ String message = t.getMessage();
+ if (StringUtils.isBlank(message) && !(t instanceof
WebApplicationException) && t.getCause() == null) {
+ String simpleName = t.getClass().getSimpleName();
+ return simpleName.isEmpty() ? t.getClass().getName() : simpleName;
+ }
+ return ExceptionUtils.appendCauses(message, t.getCause(), MAX_CAUSES,
MAX_CAUSE_LENGTH);
+ }
+
+ private boolean shouldIncludeCauses() {
+ if (_includeCauses != null) {
+ return _includeCauses;
+ }
+ if (_application != null) {
+ Object config =
_application.getProperties().get(ControllerAdminApiApplication.PINOT_CONFIGURATION);
+ if (config instanceof ControllerConf) {
+ return ((ControllerConf) config).isApiErrorResponseIncludeCauses();
+ }
+ }
+ return ControllerConf.DEFAULT_API_ERROR_RESPONSE_INCLUDE_CAUSES;
Review Comment:
Fixed in 70d6fa56ab: removed along with the switch, so there is no config
lookup left to fail.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ExceptionUtils.java:
##########
@@ -59,6 +68,128 @@ public static String consolidateExceptionMessages(Throwable
e) {
return sb.toString();
}
+ /// Appends the messages of `cause` and its causes to `message`, each after
`" -> "`. Messages already in the text are
+ /// skipped, at most `maxCauses` are kept (the outermost and the innermost,
never dropping the root cause), and each
+ /// is abbreviated in the middle to `maxCauseLength` characters.
+ @Nullable
+ public static String appendCauses(@Nullable String message, @Nullable
Throwable cause, int maxCauses,
Review Comment:
Fixed in 70d6fa56ab. Only dedup came from a real response: the ingest
wrapper embeds its cause's message, as 148 of the 463
`ControllerApplicationException` sites do, so the first cause printed twice. It
is now dedup plus one length bound; whole-word matching, surrogate handling and
the cause-count window are gone.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/ExceptionUtilsTest.java:
##########
@@ -0,0 +1,319 @@
+/**
+ * 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.pinot.common.utils;
+
+import java.io.IOException;
+import org.apache.commons.lang3.StringUtils;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertSame;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.expectThrows;
+
+
+/// Tests for [ExceptionUtils#appendCauses(String, Throwable, int, int)].
+public class ExceptionUtilsTest {
+ private static final int MAX_CAUSES = 5;
+ private static final int MAX_CAUSE_LENGTH = 1024;
+
+ @Test
+ public void testAppendCausesReturnsTheMessageWhenThereIsNoCause() {
+ String message = "Table: foo_OFFLINE not found";
+
+ assertSame(append(message, null), message);
+ assertNull(append(null, null));
+ }
+
+ @Test
+ public void testAppendCausesListsCausesOutermostFirst() {
+ Exception cause = new IllegalStateException("middle", new
IOException("inner"));
+
+ assertEquals(append("outer", cause), "outer -> middle -> inner");
+ }
+
+ @Test
+ public void testAppendCausesKeepsTheRootCauseOfASegmentCreationFailure() {
+ Exception rootCause = new IllegalArgumentException(
+ "Cannot read single-value from Object[]: [before, after] for column:
New_Value");
+ Exception transformFailure =
+ new RuntimeException("Caught exception while transforming data type
for column: New_Value", rootCause);
+ Exception readFailure = new RuntimeException("Caught exception while
reading data", transformFailure);
+ String message = "Caught exception when ingesting file into table:
foo_OFFLINE. " + readFailure.getMessage();
+
+ assertEquals(append(message, readFailure), message
+ + " -> Caught exception while transforming data type for column:
New_Value"
+ + " -> Cannot read single-value from Object[]: [before, after] for
column: New_Value");
+ }
+
+ @Test
+ public void testAppendCausesWithoutMessageStartsWithTheFirstCause() {
+ Exception cause = new IllegalStateException("middle", new
IOException("inner"));
+
+ assertEquals(append(null, cause), "middle -> inner");
+ assertEquals(append("", cause), "middle -> inner");
+ }
+
+ @Test
+ public void testAppendCausesSkipsCauseThatTheMessageAlreadyContains() {
+ Exception cause = new IOException("Connection refused");
+
+ assertEquals(append("Failed to fetch segment: Connection refused", cause),
+ "Failed to fetch segment: Connection refused");
+ }
+
+ @Test
+ public void testAppendCausesSkipsCauseThatAnEarlierCauseAlreadyContains() {
+ Exception cause = new IllegalStateException("Failed to read schema:
Unexpected character at line 3",
+ new IllegalStateException("Unexpected character at line 3", new
IOException("Stream closed")));
+
+ assertEquals(append("Invalid schema", cause),
+ "Invalid schema -> Failed to read schema: Unexpected character at line
3 -> Stream closed");
+ }
+
+ @Test
+ public void testAppendCausesCollapsesRepeatedMessages() {
+ Exception cause = new IllegalStateException("Segment push failed",
+ new IllegalStateException("Segment push failed", new
IOException("Connection reset")));
+
+ assertEquals(append("Segment push failed", cause), "Segment push failed ->
Connection reset");
+ }
+
+ @Test
+ public void testAppendCausesDropsRepeatsBeforeApplyingTheBound() {
+ Exception cause = new IllegalStateException("A", new
IllegalStateException("B", new IllegalStateException("C",
+ new IllegalStateException("D", new IllegalStateException("M", new
IllegalStateException("M"))))));
+
+ assertEquals(append("top", cause), "top -> A -> B -> C -> D -> M");
+ }
+
+ @Test
+ public void
testAppendCausesDropsCausesThatThePreviousOneSaysBeforeApplyingTheBound() {
+ Exception cause = new IllegalStateException("Failed to read row: X", new
IllegalStateException("X",
+ new IllegalStateException("Y", new IllegalStateException("Z", new
IllegalStateException("root")))));
+
+ assertEquals(ExceptionUtils.appendCauses("top", cause, 4,
MAX_CAUSE_LENGTH),
+ "top -> Failed to read row: X -> Y -> Z -> root");
+ }
+
+ @Test
+ public void testAppendCausesMatchesWholeMessagesOnly() {
+ assertEquals(append("Failed to load table_5", new IOException("5")),
"Failed to load table_5 -> 5");
+ assertEquals(append("Failed to load table: foo", new
IOException("foo_bar")),
+ "Failed to load table: foo -> foo_bar");
+ assertEquals(append("Retry limit reached: 5", new IOException("5")),
"Retry limit reached: 5");
Review Comment:
Fixed in 70d6fa56ab: removed. Dedup only skips a cause whose whole message
is already in the text, so no text is lost, only the arrow.
--
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]
