[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[ https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15609209#comment-15609209 ] Siddharth Seth commented on TEZ-3437: - Committed to branch-0.8 and branch-0.7. Thanks [~kshukla] > 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 > Fix For: 0.7.2, 0.9.0, 0.8.5 > > Attachments: TEZ-3437.001.patch, TEZ-3437.002.patch, > TEZ-3437.003.patch, TEZ-3437.004.patch, TEZ-3437.005.patch, > TEZ-3437.006.patch, TEZ-3437.007.patch, TEZ-3437.008.patch, > TEZ-3437.009.patch, TEZ-3437.branch-0.7.001.patch, > TEZ-3437.branch-0.8.001.patch > > > Follow up from TEZ-3317 to improve the getProgress thread synchronization and > replace timerTasks with ScheduledExecutorService. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[ https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15606250#comment-15606250 ] Siddharth Seth commented on TEZ-3437: - [~kshukla] - could you please post rebased versions of this patch, and TEZ-3317 for branch-0.8 and branch-0.7. The current patches don't cherry-pick cleanly to the branches. > 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 > Fix For: 0.9.0 > > Attachments: TEZ-3437.001.patch, TEZ-3437.002.patch, > TEZ-3437.003.patch, TEZ-3437.004.patch, TEZ-3437.005.patch, > TEZ-3437.006.patch, TEZ-3437.007.patch, TEZ-3437.008.patch, TEZ-3437.009.patch > > > Follow up from TEZ-3317 to improve the getProgress thread synchronization and > replace timerTasks with ScheduledExecutorService. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[ https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15606050#comment-15606050 ] Kuhu Shukla commented on TEZ-3437: -- [~sseth], Appreciate your thoughts on a possible backport of these JIRAs to 0.7. Thanks a lot! > 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 > Fix For: 0.9.0 > > Attachments: TEZ-3437.001.patch, TEZ-3437.002.patch, > TEZ-3437.003.patch, TEZ-3437.004.patch, TEZ-3437.005.patch, > TEZ-3437.006.patch, TEZ-3437.007.patch, TEZ-3437.008.patch, TEZ-3437.009.patch > > > Follow up from TEZ-3317 to improve the getProgress thread synchronization and > replace timerTasks with ScheduledExecutorService. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[ https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15566343#comment-15566343 ] Kuhu Shukla commented on TEZ-3437: -- [~sseth], Thanks a lot for the reviews and commit. Would we want to pull this commit (along with TEZ-3317) to branch-0.7? > 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 > Fix For: 0.9.0 > > Attachments: TEZ-3437.001.patch, TEZ-3437.002.patch, > TEZ-3437.003.patch, TEZ-3437.004.patch, TEZ-3437.005.patch, > TEZ-3437.006.patch, TEZ-3437.007.patch, TEZ-3437.008.patch, TEZ-3437.009.patch > > > Follow up from TEZ-3317 to improve the getProgress thread synchronization and > replace timerTasks with ScheduledExecutorService. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[ https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=1680#comment-1680 ] Siddharth Seth commented on TEZ-3437: - +1. Committing. Thanks [~kshukla] > 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, TEZ-3437.004.patch, TEZ-3437.005.patch, > TEZ-3437.006.patch, TEZ-3437.007.patch, TEZ-3437.008.patch, TEZ-3437.009.patch > > > Follow up from TEZ-3317 to improve the getProgress thread synchronization and > replace timerTasks with ScheduledExecutorService. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15549974#comment-15549974
]
Kuhu Shukla commented on TEZ-3437:
--
{code}
!?
/home/jenkins/jenkins-slave/workspace/PreCommit-TEZ-Build/tez-runtime-library/src/test/resources/TestIFile_concatenated_compressed.bin
Lines that start with ? in the release audit report indicate files that do
not have an Apache license header.
{code}
This is an unrelated warning.
> 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, TEZ-3437.004.patch, TEZ-3437.005.patch,
> TEZ-3437.006.patch, TEZ-3437.007.patch, TEZ-3437.008.patch, TEZ-3437.009.patch
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15549935#comment-15549935
]
TezQA commented on TEZ-3437:
{color:red}-1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12831802/TEZ-3437.009.patch
against master revision 149db1b.
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 3 new
or modified test files.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 javadoc{color}. There were no new javadoc warning messages.
{color:green}+1 findbugs{color}. The patch does not introduce any new
Findbugs (version 3.0.1) warnings.
{color:red}-1 release audit{color}. The applied patch generated 1
release audit warnings.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-TEZ-Build/2013//testReport/
Release audit warnings:
https://builds.apache.org/job/PreCommit-TEZ-Build/2013//artifact/patchprocess/patchReleaseAuditProblems.txt
Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/2013//console
This message is automatically generated.
> 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, TEZ-3437.004.patch, TEZ-3437.005.patch,
> TEZ-3437.006.patch, TEZ-3437.007.patch, TEZ-3437.008.patch, TEZ-3437.009.patch
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15549450#comment-15549450
]
Siddharth Seth commented on TEZ-3437:
-
{code}
+ public float getProgress() throws ProgressFailedException,
InterruptedException {
+try {
+ return (mrReader != null) ? mrReader.getProgress() : 0.0f;
+} catch (IOException e) {
+ throw new ProgressFailedException("getProgress encountered IOException
", e);
+} catch (InterruptedException e) {
+ throw new ProgressFailedException("getProgress was Interrupted ", e);
+}
}
{code}
Why catch an InterruptedException here? The method signature already declares
this as being throwable.
Signature of getProgrress in ConcatenatedMergedKeyValuesInput... this is also
catching InterruptedException. [~kshukla] - could you please look at all
implementations of getProgress for correct signatures, exception handling in
the next patch.
{code}
+ public void shutDownProgressTaskService() {
+scheduledExecutorService.shutdownNow();
+ }
{code}
Also requires a null check (The variable is setup on invocation of a method,
outside the constructor). In fact schedulerExecutorService should be volatile.
Other than these comments - looks ok to me from a quick glance.
> 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, TEZ-3437.004.patch, TEZ-3437.005.patch,
> TEZ-3437.006.patch, TEZ-3437.007.patch, TEZ-3437.008.patch
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537262#comment-15537262
]
TezQA commented on TEZ-3437:
{color:green}+1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12831163/TEZ-3437.008.patch
against master revision 5c2f893.
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 3 new
or modified test files.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 javadoc{color}. There were no new javadoc warning messages.
{color:green}+1 findbugs{color}. The patch does not introduce any new
Findbugs (version 3.0.1) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-TEZ-Build/2004//testReport/
Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/2004//console
This message is automatically generated.
> 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, TEZ-3437.004.patch, TEZ-3437.005.patch,
> TEZ-3437.006.patch, TEZ-3437.007.patch, TEZ-3437.008.patch
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15536732#comment-15536732
]
Kuhu Shukla commented on TEZ-3437:
--
{quote}
Thinking about this some more, I really don't like the idea of a background
thread taking care of reporting progress every little bit of time. It's very
easy to forget to shutdown this thread. It'll be a lot better if each processor
that wants to report progress does so after n tuples are processed. I'd expect
Hive and Pig processors would go with this approach (ref
getContext().notifyProgress usage).
Lets get this patch in with these fixes, and see if anything needs to change
after.
{quote}
[~sseth] I agree, all readers and KV processing calls notifyProgressInvocation.
But does that mean that we keep the latest change to setProgress as is, which
is not decouple the idea of "set value of progress" from "notify progress".
Will update the patch for other comments today.
> 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, TEZ-3437.004.patch, TEZ-3437.005.patch,
> TEZ-3437.006.patch, TEZ-3437.007.patch
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[ https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15536522#comment-15536522 ] Siddharth Seth commented on TEZ-3437: - bq. Thank you so much Siddharth Seth for the quick and detailed comments. I wouldn't really call that quick ... Thanks for the patience, and patch iterations. bq. Also, for adding InterruptedException to the interface, I added it to the getProgress call in AbstractLogicalInput/MergedLogicalInput. Is that what you meant? This needs some changes elsewhere as well though. e.g. MRInput.getProgress() signature can be changed to throw InterruptedException. Could you please look through other Inputs where this change may be relevant. FilterByWordInputProcessor does not use progressHelper ... this is a NPE waiting to happen. Looks like none of the tests catch it. progressHelper is typically setup in the run method. A close can be invoked before run is called (i.e. after the initialize). All such close checks need to be wrapped in a null check. Possible to pass in a name to the ProgressHelper? so that each such thread does not have a generic name, instead it gets a name based on the Processor being used (or whatever context is passed in). Thinking about this some more, I really don't like the idea of a background thread taking care of reporting progress every little bit of time. It's very easy to forget to shutdown this thread. It'll be a lot better if each processor that wants to report progress does so after n tuples are processed. I'd expect Hive and Pig processors would go with this approach (ref getContext().notifyProgress usage). Lets get this patch in with these fixes, and see if anything needs to change after. cc [~jlowe], [~hitesh] for comments. > 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, TEZ-3437.004.patch, TEZ-3437.005.patch, > TEZ-3437.006.patch, TEZ-3437.007.patch > > > Follow up from TEZ-3317 to improve the getProgress thread synchronization and > replace timerTasks with ScheduledExecutorService. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15534831#comment-15534831
]
TezQA commented on TEZ-3437:
{color:green}+1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12831032/TEZ-3437.007.patch
against master revision 5c2f893.
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 3 new
or modified test files.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 javadoc{color}. There were no new javadoc warning messages.
{color:green}+1 findbugs{color}. The patch does not introduce any new
Findbugs (version 3.0.1) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-TEZ-Build/1997//testReport/
Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/1997//console
This message is automatically generated.
> 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, TEZ-3437.004.patch, TEZ-3437.005.patch,
> TEZ-3437.006.patch, TEZ-3437.007.patch
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[ https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15534366#comment-15534366 ] Siddharth Seth commented on TEZ-3437: - On the setProgress bit - setProgress() is mainly used for keep alive. The change made earlier - which invokes setProgress() makes sense. Apologies for the confusion caused by previous comments. The float check there however, likely needs to be changed. Equality on float is not great. > 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, TEZ-3437.004.patch, TEZ-3437.005.patch, TEZ-3437.006.patch > > > Follow up from TEZ-3317 to improve the getProgress thread synchronization and > replace timerTasks with ScheduledExecutorService. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15534341#comment-15534341
]
Siddharth Seth commented on TEZ-3437:
-
- On the interface, can we include InterruptedException as well?
- Think this bit was missed out in the consolidation - MapProcessor is creating
the same Runnable that is created in ProgressHelper
- Bunch of getters/setters can be removed from ProgressHelper after this, and
some final fields? | Or was this MapProcessor change done for some reason?
- Nit: Name the thread in ProgressHelper based on the invoker, set as daemon
(ThreadFactoryBuilder)
- {code}
+if (e.getCause() instanceof IOException) {
+ throw new IOException(e);
+}
{code} Cast to IOException, instead of creating a new IOException.Likewise for
InterruptedException.
Same place - instead of returning 0.0f as progress, this should throw a
RuntimException.
- TezTaskContextImpl.getRuntimeTask - why is this required? (Don't think it's
used anywhere)
- Nit: private AtomicLong totalBytesRead = new AtomicLong(0); - should be
final. Also other similar fields.
> 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, TEZ-3437.004.patch, TEZ-3437.005.patch, TEZ-3437.006.patch
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[ https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15530140#comment-15530140 ] Kuhu Shukla commented on TEZ-3437: -- [~sseth], Request for comments/review. Thank you! > 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, TEZ-3437.004.patch, TEZ-3437.005.patch, TEZ-3437.006.patch > > > Follow up from TEZ-3317 to improve the getProgress thread synchronization and > replace timerTasks with ScheduledExecutorService. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15511190#comment-15511190
]
TezQA commented on TEZ-3437:
{color:green}+1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12829662/TEZ-3437.006.patch
against master revision da4098b.
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 3 new
or modified test files.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 javadoc{color}. There were no new javadoc warning messages.
{color:green}+1 findbugs{color}. The patch does not introduce any new
Findbugs (version 3.0.1) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-TEZ-Build/1982//testReport/
Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/1982//console
This message is automatically generated.
> 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, TEZ-3437.004.patch, TEZ-3437.005.patch, TEZ-3437.006.patch
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15510955#comment-15510955
]
TezQA commented on TEZ-3437:
{color:red}-1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12829634/TEZ-3437.005.patch
against master revision da4098b.
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 3 new
or modified test files.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 javadoc{color}. There were no new javadoc warning messages.
{color:green}+1 findbugs{color}. The patch does not introduce any new
Findbugs (version 3.0.1) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:red}-1 core tests{color}. The patch failed these unit tests in :
org.apache.tez.dag.app.rm.TestContainerReuse
org.apache.tez.mapreduce.processor.map.TestMapProcessor
Test results:
https://builds.apache.org/job/PreCommit-TEZ-Build/1980//testReport/
Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/1980//console
This message is automatically generated.
> 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, TEZ-3437.004.patch, TEZ-3437.005.patch, TEZ-3437.006.patch
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15508585#comment-15508585
]
TezQA commented on TEZ-3437:
{color:green}+1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12829484/TEZ-3437.004.patch
against master revision 9cf25d1.
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 3 new
or modified test files.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 javadoc{color}. There were no new javadoc warning messages.
{color:green}+1 findbugs{color}. The patch does not introduce any new
Findbugs (version 3.0.1) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-TEZ-Build/1977//testReport/
Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/1977//console
This message is automatically generated.
> 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, TEZ-3437.004.patch
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15508301#comment-15508301
]
Kuhu Shukla commented on TEZ-3437:
--
Something like this :
TezProcessorContextImpl :
{code}
@Override
public void setProgress(float progress) {
runtimeTask.setProgress(progress);
}
{code}
To be consistent we would need to add the following to MRTaskReporter :
{code}
if (isProcessorContext) {
((ProcessorContext)context).setProgress(progress);
context.notifyProgress();
}
{code}
This would mean that setProgress does not auto-invoke notifyProgress after this
change. Would that be acceptable?
Another way would have been as I mentioned above, have runtimeTask available to
the progress thread, which is not straight forward since we use
ProcessorContext objects (an interface) and type casting to
{{TezProcessorContextImpl}} breaks dependency rules and would be restrictive.
> 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
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15508249#comment-15508249
]
Kuhu Shukla commented on TEZ-3437:
--
Thanks a lot for the review [~sseth].
{quote}
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.
{quote}
Since we already have the lazySet for the AtomicBoolean for progress update,
how about we just do the float value set through the processor thread
invocation and let the {{notifyProgress()}} and
{{ProcessorContext#setProgress}} remain as it is? I can add a getter for
{{runtimeTask}} in {{TezTaskContextImpl}} and call
{{LogicalIOProcessorRuntimeTask#setProgress()}} directly in
{{ProgressHelper#monitorProgress}} thread. Thoughts?
I will address other comments in the upcoming patch.
> 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
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15507984#comment-15507984
]
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.
{code}
+if (runtimeTask.getProgress() != progress) {
+ runtimeTask.setProgress(progress);
+ notifyProgress();
+}
{code}
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
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[ https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15506878#comment-15506878 ] Kuhu Shukla commented on TEZ-3437: -- [~sseth], Request for comments/review on the patch [and questions above|https://issues.apache.org/jira/browse/TEZ-3437?focusedCommentId=15497312=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15497312]. Thank you! > 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 (v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[ https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15502181#comment-15502181 ] Kuhu Shukla commented on TEZ-3437: -- [~sseth], Request for comments/review on the patch [and questions above|https://issues.apache.org/jira/browse/TEZ-3437?focusedCommentId=15497312=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15497312]. Thanks a lot! > 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 (v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15502172#comment-15502172
]
TezQA commented on TEZ-3437:
{color:green}+1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12829129/TEZ-3437.003.patch
against master revision b17edc4.
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 3 new
or modified test files.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 javadoc{color}. There were no new javadoc warning messages.
{color:green}+1 findbugs{color}. The patch does not introduce any new
Findbugs (version 3.0.1) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-TEZ-Build/1974//testReport/
Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/1974//console
This message is automatically generated.
> 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
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15501985#comment-15501985
]
TezQA commented on TEZ-3437:
{color:red}-1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12829124/TEZ-3437.002.patch
against master revision b17edc4.
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 3 new
or modified test files.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 javadoc{color}. There were no new javadoc warning messages.
{color:red}-1 findbugs{color}. The patch appears to introduce 3 new
Findbugs (version 3.0.1) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-TEZ-Build/1973//testReport/
Findbugs warnings:
https://builds.apache.org/job/PreCommit-TEZ-Build/1973//artifact/patchprocess/newPatchFindbugsWarningstez-mapreduce.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-TEZ-Build/1973//artifact/patchprocess/newPatchFindbugsWarningstez-runtime-library.html
Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/1973//console
This message is automatically generated.
> 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
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (TEZ-3437) Improve synchronization and the progress report behavior for Inputs from TEZ-3317
[
https://issues.apache.org/jira/browse/TEZ-3437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15497570#comment-15497570
]
TezQA commented on TEZ-3437:
{color:red}-1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12828897/TEZ-3437.001.patch
against master revision b17edc4.
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 3 new
or modified test files.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 javadoc{color}. There were no new javadoc warning messages.
{color:red}-1 findbugs{color}. The patch appears to introduce 4 new
Findbugs (version 3.0.1) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-TEZ-Build/1972//testReport/
Findbugs warnings:
https://builds.apache.org/job/PreCommit-TEZ-Build/1972//artifact/patchprocess/newPatchFindbugsWarningstez-mapreduce.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-TEZ-Build/1972//artifact/patchprocess/newPatchFindbugsWarningstez-runtime-library.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-TEZ-Build/1972//artifact/patchprocess/newPatchFindbugsWarningstez-api.html
Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/1972//console
This message is automatically generated.
> 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
>
>
> Follow up from TEZ-3317 to improve the getProgress thread synchronization and
> replace timerTasks with ScheduledExecutorService.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
