[SPARK-55240][CORE] Refactor LazyTry stacktrace handling to use wrapper instead of suppressed exception #54007
Conversation
JIRA Issue Information=== Improvement SPARK-55240 === This comment was automatically generated by GitHub Actions |
cloud-fan
changed the title
|
This is kind of followup of #48882 |
| // Stitch the stacktrace: keep the "below" part, replace "above" with current caller | ||
| originalEx.setStackTrace( |
There was a problem hiding this comment.
IIUC, at this point, the original above stack trace is lost, and also it's indistinguishable for the user, whether the exception happened here, or was only retrieved and stitched from an earlier call?
I do find it useful for debugging to know if the exception really happened on the stack trace of the current call, or it happened in a different call and it was only stitched to pretend as happening here, while in fact it was retrieved from a Try....
I like the wrapper change that makes it cleaner than searching for OriginalTryStackTraceException among the supressed, but I personally would prefer to preserve and show the information about the original place the exception was thrown... However, I can not think of a different way to add it than again adding it as a suppressed Exception, which would then have both a wrapper and a suppressed exception, so I don't think it would be an improvement to the current state...
There was a problem hiding this comment.
The suppressed Exception is not ideal, because it's more the "cause", but we cannot make it the cause, because the original exception might have already been the cause... Having it as suppressed exception was the only way I could find to tag it along without disrupting the original exception...
There was a problem hiding this comment.
The more common case is people see the error once and fix the query. It's better to optimize for the common case (no suppressed error is needed).
I agree that it's useful to know if the error was caused previously. Shall we insert a special frame in the error stacktrace to indicate it? e.g.
original error stacktrace
...
doTryWithCallerStacktrace (only present if it's not the first invocation)
...
current caller stacktrace
There was a problem hiding this comment.
They would still see the error message on top and their call stacktrace, and probably ignore the suppressed listing below.
In practical cases where I rely on it I would most likely be connected with a debugger anyway and then can get the original caller trace anyway... But if it's a bug report with no repro but only the log, the suppressed part can be useful in providing some triage... From user perspective it is indeed a longer and uglier error dump, something that @yaooqinn complained about in the past.
For me the suppressed full original trace is useful, and only a small ugliness for a casual end user, but I don't insist.
| val originalEx = wrapper.getCause | ||
| // Stitch the stacktrace: keep the "below" part, replace "above" with current caller | ||
| originalEx.setStackTrace( | ||
| wrapper.belowStacktrace ++ Thread.currentThread().getStackTrace.drop(1)) |
There was a problem hiding this comment.
if going ahead with it, you don't need to store belowStacktrace at all, only the prefix length. The below part is already in originalEx, and is not changing, only the suffix of the current caller changes.
So
val belowStacktraceLength = origStackTrace.size - commonSuffixLen
in doTryWithCallerStacktrace and here
originalEx.setStackTrace(
orignalEx.take(belowStacktraceLength) ++ Thread.currentThread().getStackTrace.drop(1))
should work.
| // We are modifying and throwing the original exception. It would be better if we could | ||
| // return a copy, but we can't easily clone it and preserve. If this is accessed from | ||
| // multiple threads that then look at the stack trace, this could break. |
There was a problem hiding this comment.
I would preserve this code comment next to stack stitching because it still applies.
…er instead of suppressed exception This PR refactors how `doTryWithCallerStacktrace` and `getTryWithCallerStacktrace` handle stacktrace stitching. Instead of adding an `OriginalTryStackTraceException` as a suppressed exception on the original exception, we now wrap the original exception inside `OriginalTryStackTraceException`. Key changes: - `OriginalTryStackTraceException` now wraps the original exception (as its cause) and stores the pre-computed "below" stacktrace portion - `doTryWithCallerStacktrace` returns `Failure(wrapper)` instead of modifying the original exception's suppressed list - `getTryWithCallerStacktrace` unwraps the original exception, stitches the stacktrace, and throws the original (not the wrapper) Benefits: - Cleaner exception output - no suppressed exceptions visible to users - Simpler code - no need to track first access or manage suppressed exception lifecycle - Original exception type is preserved when thrown
juliuszsompolski
left a comment
juliuszsompolski
left a comment
There was a problem hiding this comment.
LGTM, small nit.
| val isFirstAccess = !materialized | ||
| materialized = true | ||
| Utils.getTryWithCallerStacktrace(tryT, isFirstAccess) |
There was a problem hiding this comment.
nit: I don't think it matters much as there are some atomicity problems with resetting the trace anyway, but maybe use AtomicBoolean and just call Utils.getTryWithCallerStacktrace(tryT, !materialized.getAndSet(true))
There was a problem hiding this comment.
it's not thread-safe anyway, and AtomicBoolean takes more memory.
2 participants
What changes were proposed in this pull request?
This PR refactors how
doTryWithCallerStacktraceandgetTryWithCallerStacktracehandle stacktrace stitching. Instead of adding anOriginalTryStackTraceExceptionas a suppressed exception on the original exception, we now wrap the original exception insideOriginalTryStackTraceException.Key changes:
OriginalTryStackTraceExceptionnow wraps the original exception (as its cause) and stores the pre-computed "below" stacktrace portiondoTryWithCallerStacktracereturnsFailure(wrapper)instead of modifying the original exception's suppressed listgetTryWithCallerStacktraceunwraps the original exception, stitches the stacktrace, and throws the original (not the wrapper)Why are the changes needed?
no suppressed exceptions visible to users
Does this PR introduce any user-facing change?
Yes, exceptions thrown from
LazyTry.getwill no longer have a suppressedOriginalTryStackTraceException. The stacktrace stitching behavior remains the same.How was this patch tested?
Existing unit tests in
UtilsSuiteandLazyTrySuiteupdated and passing.Was this patch authored or co-authored using generative AI tooling?
Yes.