[GitHub] [druid] gianm commented on a diff in pull request #14171: Adding EitherException to prevent nested error in Either usage.

["gianm (via GitHub)" <gi...@apache.org>]

gianm commented on code in PR #14171:
URL: https://github.com/apache/druid/pull/14171#discussion_r1194386452


##########
processing/src/main/java/org/apache/druid/java/util/common/Either.java:
##########
@@ -77,33 +77,50 @@ public L error()
   }
 
   /**
-   * If this Either represents a value, returns it. If this Either represents an error, throw an error.
-   *
-   * If the error is a {@link Throwable}, it is wrapped in a RuntimeException and thrown. If it is not a throwable,
-   * a generic error is thrown containing the string representation of the error object.
-   *
+   * <ul>
+   *  <li>
+   *  If this Either represents a value, returns it.
+   *  </li>
+   *  <li>
+   *  If this Either represents an error, throw an error.
+   *  <ul>
+   *    <li>
+   *    If the error is an {@link EitherException}, the original exception is thrown.
+   *    </li>
+   *    <li>
+   *    If the error is a {@link Throwable}, it is wrapped in an {@link EitherException} and thrown.
+   *    </li>
+   *    <li>
+   *    If it is not a throwable, a generic {@link EitherException} is thrown containing the string representation of the error object.
+   *    </li>
+   *    </li>
+   *  </ul>
+   * </ul>
+   * <br/>
    * To retrieve the error as-is, use {@link #isError()} and {@link #error()} instead.
    */
   @Nullable
   public R valueOrThrow()
   {
     if (isValue()) {
       return value;
+    } else if (error instanceof EitherException) {
+      throw (EitherException) error;

Review Comment:
   I don't think Either should be assuming that people want the exception stack collapsed. It doesn't really know how it's going to be used. Better, IMO to do the following:
   
   - Have this code always wrap in EitherException, even if `error` is an EitherException itself. (Ensures we always have the full exception stack trace available, even when it's passed around and rethrown.)
   - Have the code that reports MSQ errors unwrap the EitherExceptions. This code knows it only cares about the "inner" exception.



-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org