Siddharth Seth commented on TEZ-3437:

bq. Yes. For 0 inputs how should the progress be reported?
1.0f - always complete.

bq. Fixed method signatures to be consistent. Let me know if I am missing 
something here.
Individual inputs don't need to throw the Exception. The question was really 
around why IOException and InterruptedException. I'd change this to either 
Exception - similar to the other methods (but not very useful), or 
InterruptedException + a TezCheckedException or a custom 
ProgressFailedException. Anyone else has thoughts on this? Processors may want 
to handle this, hence a checked Exception.

bq. My assumption here is that totalBytesRead and totalFileBytes will be used 
as and when available. As a background thread for progress update, should it 
matter that the read and writes to progress value is not synchronized? 
Appreciate some more comments on how we can make this better and what would you 
do to fix this.
They're read and updated in different threads. It's possible for the reading 
thread to always see a cached copy. Could use Atomic vars to fix this, or 
progress specific locks. This is mainly related to visibility.

bq. Since some processors dont have the conf objects, should we make a more 
elaborate change for the processors to pick up conf as well(add conf as a 
member maybe?).
Most of the ones which don't have a config are test processors. I don't think 
they need change. I'm curious about Pig and Hive will handle this though - 
hardcode to 100ms, or? What does the framework recommend.

+    if (runtimeTask.getProgress() != progress) {
+      runtimeTask.setProgress(progress);
+      notifyProgress();
+    }
Not sure why this change was required in the original patch? Can we let 
progress reporting continue as is? This progress is used as a heartbeat for the 
task timeout monitor. (TezConfiguration#TEZ_TASK_PROGRESS_STUCK_INTERVAL_MS).
Earlier, this timeout handling would rely upon the processor explicitly setting 
progress - which would typically be done after n rows. That's no longer the 
case. Even if the processor keeps reporting progress - 0.0f for a long time, 
while it waits for instance, it'll end up timing out.
With the check completely removed - we have a thread running which reports 
progress - and makes the processor 'report after n entries' fairly useless.

The extra thread to report progress works nicely for the test processors. For 
Pig, and Hive - I'd imagine they'd want progress to be reported after a certain 
number of entries are processed - rather than a heartbeat thread.

Any thoughts on reporting progress based on Outputs?

> Improve synchronization and the progress report behavior for Inputs from 
> TEZ-3317
> ---------------------------------------------------------------------------------
>                 Key: TEZ-3437
>                 URL: https://issues.apache.org/jira/browse/TEZ-3437
>             Project: Apache Tez
>          Issue Type: Bug
>            Reporter: Kuhu Shukla
>            Assignee: Kuhu Shukla
>         Attachments: TEZ-3437.001.patch, TEZ-3437.002.patch, 
> TEZ-3437.003.patch
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and 
> replace timerTasks with ScheduledExecutorService. 

This message was sent by Atlassian JIRA

Reply via email to