Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3583
@tillrohrmann Thanks for the review, will add the null check to
`failGlobal` and merge it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as we
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3583
I've rebased the branch, @tillrohrmann could you check it out again for the
changes in the ExecutionGraph?
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3583
@tillrohrmann I've updated the PR. The changes to the EG we're reduced to a
minimum; `ExecutionGraph#fail()/suspend()` now take an `ErrorInfo` instead of a
`Throwable`. The old methods were kept to no
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3583
@tillrohrmann What do you think about passing an ErrorInfo instead of a
Throwable so that we don't need the additional timestamp argument?
---
If your project is set up for it, you can reply to this
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/3583
It's not per se horrible to pass on the timestamp. In my opinion it's just
a good idea trying to keep the interface as lean as possible to avoid making
mistakes. If we say that we want to displa
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3583
Of course it wouldn't be the end of the world to display an inconsistent
timestamp, it just doesn't make sense for them to be different though. The root
exception display is only meant to highlight th
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3583
It's inconsistent, that's all I'm saying.
Passing an ErrorInfo instead of a Throwable is also useful (probably even
required) for FLINK-6042 since it allows us to attach meta data to the
exce
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/3583
Wouldn't it be ok, if the timestamps differed slightly? I mean the EG root
cause timestamp is the time when the EG was failed, whereas the `Execution`
related timestamp says when the failure at
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3583
@tillrohrmann I've addressed your comments regarding ErrorInfo being
volatile/fields being final as well as markTimestamp generating and returning
the timestamp.
@StephanEwen @tillrohrmann Re
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3583
Can we remove the `errorTimestamp` from the `notifyStateTransition`? I
think it obfuscates the meaning a bit...
---
If your project is set up for it, you can reply to this email and have your
re
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3583
The point of the ErrorInfo is to actually enforce that the exception is
always set with a timestamp. By adding a method you enforce nothing, the
exception can still be set without a timesmtap.
---
I
Github user uce commented on the issue:
https://github.com/apache/flink/pull/3583
Thanks for addressing the comments. I'm unsure about the `ErrorInfo`
instead of adding the suggested method. Since @tillrohrmann and @StephanEwen
often work on the `ExecutionGraph` I would like their inp
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3583
@uce I've addressed your comments. The timestamp box will now be hidden if
the timestamp wasn't visible yet. For the exception stuff in the EG I've
introduce a separate container class ```ErrorInfo```
13 matches
Mail list logo
