[
https://issues.apache.org/jira/browse/TEZ-4103?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16987349#comment-16987349
]
Jonathan Turner Eagles commented on TEZ-4103:
---------------------------------------------
Couple of very minor misspellings and word clarity
* periodicMonitoTaskRef should be periodicMonitorTaskRef
* inpuProgress should be inputProgress
* Vertex Progress should be Vertex progress
* "correct range" might be more clear as "valid range"
There were a number of functions in ProgressHelper that were moved as well as
changes that weren't necessary but it's probably better to clean them up as you
did.
Let changes below debug lines to parameterized LOG.debug lines instead.
http://www.slf4j.org/faq.html#logging_performance
{code}
+ LOG.debug(String.format(
+ "Incorrect progress in progress helper in processor=%s, "
+ + "inpuProgress=%f, inputsSize=%d, invalidInput=%d",
+ processorName, inputProgress, inputs.size(),
+ invalidSnapshot));
+ LOG.debug(String.format("Vertex Progress not in correct range: "
+ + "v=%s, progress=%f", v.getName(), vertexProgress));
+ LOG.debug(String.format(
+ "vertex=%s, task=%s: Progress not in correct range=%f",
+ getName(), task.getTaskId().toString(), taskProg));
+ }
{code}
As far as design goes, there are a couple of things.
1) DAGImpl now validates that all VertexImpl progress values are in the valid
range. The change also validates the VetexImpl inputs so that only valid
progress values are returned in getProgress. So we are checking the validity
twice.
2) TaskAttemptImpl.getProgress() is the input to the speculator and if
Processor uses ProgressHelper then it is protected, if not, unfiltered data is
still sent to the speculator. Does it make more sense to move all or part of
the range change to TaskAttemptImpl?
3) Logging is defaulted to debug. Does it make more sense to always log the
first invalid progress or perhaps every X invalid values? Especially useful to
track down buggy ReaderProgress.
One
pre-existing issue that seems wrong to me is that progress is modified with
only a read lock instead of with a write lock (write locks are automatically
retrieved during state machine transitions).
> Progress in DAG, Vertex, and tasks is incorrect
> -----------------------------------------------
>
> Key: TEZ-4103
> URL: https://issues.apache.org/jira/browse/TEZ-4103
> Project: Apache Tez
> Issue Type: Bug
> Reporter: Ahmed Hussein
> Assignee: Ahmed Hussein
> Priority: Major
> Attachments: TEZ-4103.001.patch, TEZ-4103.002.patch,
> TEZ-4103.003.patch, TEZ-4103.004.patch
>
>
> Looking at the progress code, there some few issues that could lead to some
> problems calculating the progress.
> There are some cases when the progress never reach 1.0.
> This is a list of issues that need to be fixed in the progress code:
> * After TEZ-3982, since values are skipped in the In some cases, the
> progress of DAG or a vertex may never reach 1.0f. this is in both
> "{{DAGImpl.java}}" and "{{ProgressHelper.java}}"
> * {{ProgressHelper}} schedules a service to update the progress, dubbed
> `{{ProgressHelper.monitorProgress}}`. According to Java Documentation:
> {quote}If any execution of the task encounters an exception,
> subsequent executions are suppressed.
> Otherwise, the task will only terminate via cancellation
> or termination of the executor.
> {quote}
> In other words, if the service dies, there is no way to catch that in the
> code and the progress will never be updated.
> * The `{{SimpleProcessor.inputMap}}` is not thread-safe. They are
> initialized as `{{LinkedHashMap}}` and there is no synchronization on the
> field objects in the map. This could be problematic in concurrent context.
> * `{{VertexImpl.getProgress()}}` does not check the range of the progress
> calculated in `{{VertexImpl.computeProgress()}}`
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
