KKcorps commented on code in PR #19716:
URL: https://github.com/apache/pinot/pull/19716#discussion_r4311663262
##########
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:
The PR marks 6 sites `LOG_ONLY` because causes can leak controller detail,
but the other ~199 now default to `FULL`. Why default on rather than opt-in per
endpoint, given #19238 just went the other way?
##########
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:
`consolidateExceptionMessages` already does most of this in 15 lines. Which
of the extra rules (word-boundary dedup, surrogate-safe trimming, outer+inner
window) came from a real response you saw, and could we start with a simpler
version?
##########
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:
This mapper catches every `Throwable`, so raw exceptions that never went
through `ControllerApplicationException` now return their causes too, with no
opt-out. Is that intended, and what's the worst cause message you can imagine
coming through here?
##########
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:
If the config lookup ever misses, this falls back to `true`. Should a switch
that guards what we expose fail closed instead?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/RecordProcessingException.java:
##########
@@ -0,0 +1,29 @@
+/**
+ * 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.segment.spi.creator;
+
+/// Thrown when segment creation fails to read or process a record of its
input.
+public class RecordProcessingException extends RuntimeException {
Review Comment:
This adds a public SPI type mainly so the REST layer can decide what's safe
to show. What's the tradeoff of putting that decision in segment-creation code
that minions and Spark jobs also run?
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -132,7 +132,7 @@ public static void validateFineGrainedAuth(Method
endpointMethod, UriInfo uriInf
String errorMsg = "Failed to check for access for target type " +
auth.targetType() + " and target ID "
+ targetId + " with action " + auth.action();
LOGGER.error(errorMsg, t);
- throw new WebApplicationException(errorMsg, t,
Response.Status.INTERNAL_SERVER_ERROR);
+ throw new WebApplicationException(errorMsg,
Response.Status.INTERNAL_SERVER_ERROR);
Review Comment:
This is in pinot-core, so it changes the broker too. Can you call it out in
the description and explain why dropping the cause is safe there?
##########
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:
This test shows a real cause (`"5"`) getting swallowed because its text
happens to appear in the message. When does whole-word dedup help more than it
hurts?
--
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]
