Kuhu Shukla updated TEZ-3437:
    Attachment: TEZ-3437.001.patch

Uploading first version of the patch addressing comments from TEZ-3317 inline 
below. [~sseth], appreciate more comments/review on the patch and questions 
below. Thanks a lot!

bq. Progress in case of 0 inputs. Not sure if this is handled (suspect it's 
returning 0.0f)
Yes. For 0 inputs how should the progress be reported?
bq. TezProcessorContextImpl - != comparison on a float. Will this work as 
bq. getProgress on the processor throws an IOException and 
InterruptedException. Why is IOException special cased?
Fixed method signatures to be consistent. Let me know if I am missing something 
bq. The Timer threads that are setup don't have any kind of synchronization. 
There's no synchronization on the processors when accessing the progress 
either. Think visibility of variables within the timed thread that reports 
progress can be a problem. e.g. totalBytesRead and totalFileBytes are updated 
by the worker thread, but read by the thread started by the timer in 

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.

bq.Does the time interval in the MapProcessor (100ms at the moment) need to be 
linked to any other config property / made configurable?
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?).
bq. [Nit] UnorderedKVReader - return 0l instead of 0.0f?
bq. TestMapProcessor - generateInputSplit, writeSplitFiles. This looks very 
similar to what exists in MapUtils. Can that be re-used?
Fixed with changes to MapUtils method signature to read the number of KV pairs 
to write.
bq. ShuffleManager.getNumCompletedInputs. Expose the int value, instead of 
exposing the internal AtomicInteger
Fixed to return float value straight up.
bq. The Processors seem to share code for reporting timed progress. Can this be 
moved into a common base, or a helper?
Fixed. Appreciate more comments.
bq. Specific reason to use a Timer? Otherwise can we move to a 

> 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
> 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