-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix profiling tool reading collectionAccumulator #5094
Conversation
Signed-off-by: Thomas Graves <tgraves@nvidia.com>
build |
@@ -404,8 +404,8 @@ class EventsProcessor(app: ApplicationInfo) extends EventProcessorBase[Applicati | |||
app.accumIdToStageId.put(res._2.id, event.stageInfo.stageId) | |||
arrBuf += thisMetric | |||
} catch { | |||
case e: ClassCastException => | |||
logWarning("ClassCastException when parsing accumulables for task " + | |||
case e: Exception => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we catch exceptions more exhaustively with matching on Throwable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's an Error, I think we just want to fail as that could be something more than just related to the accumulable, what I actually probably should do is make this match on NonFatal(e)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for NonFatal, it's consistent with similar usages in our code base
build |
The profiling tool can fail to process an event log if it has custom task accumulables of type CollectionAccumulator, this just changes it to catch the exception and ignore those accumulables.
This is a minimal fix for #5091 but I want to leave that open and handle these better.
Signed-off-by: Thomas Graves tgraves@nvidia.com