You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Rajesh Balamohan <rb...@apache.org> on 2020/05/12 21:59:09 UTC
Review Request 72503: HIVE-23449: LLAP: Reduce mkdir and config
creations in submitWork hotpath
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72503/
-----------------------------------------------------------
Review request for hive and Ashutosh Chauhan.
Repository: hive-git
Description
-------
For short jobs, submitWork() becomes hotpath. Patch tries to lazy load conf creation and also gets rid of dir creations (which needs to be enabled only when DirWatcher is enabled in ShuffleHandler)
Diffs
-----
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java 6a13b55e69
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java eae8e08540
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java 36192520e3
llap-server/src/java/org/apache/hadoop/hive/llap/shufflehandler/ShuffleHandler.java aff2c2ec39
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java 50dec4759e
Diff: https://reviews.apache.org/r/72503/diff/1/
Testing
-------
Thanks,
Rajesh Balamohan
Re: Review Request 72503: HIVE-23449: LLAP: Reduce mkdir and config
creations in submitWork hotpath
Posted by Rajesh Balamohan <rb...@apache.org>.
> On May 13, 2020, 3:03 a.m., Ashutosh Chauhan wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
> > Lines 197 (patched)
> > <https://reviews.apache.org/r/72503/diff/2/?file=2231479#file2231479line197>
> >
> > Aah I see, Make sense.
> > Can you add comment for this during commit in ContainerRunnerImpl.java for usage of supplier. As you can see, it wasnt apparent for me :)
Sure. Will do. :)
- Rajesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72503/#review220734
-----------------------------------------------------------
On May 13, 2020, 2:28 a.m., Rajesh Balamohan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72503/
> -----------------------------------------------------------
>
> (Updated May 13, 2020, 2:28 a.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> For short jobs, submitWork() becomes hotpath. Patch tries to lazy load conf creation and also gets rid of dir creations (which needs to be enabled only when DirWatcher is enabled in ShuffleHandler)
>
>
> Diffs
> -----
>
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java 6a13b55e69
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java eae8e08540
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java 36192520e3
> llap-server/src/java/org/apache/hadoop/hive/llap/shufflehandler/ShuffleHandler.java aff2c2ec39
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java 50dec4759e
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestContainerRunnerImpl.java 8ae00b9c87
>
>
> Diff: https://reviews.apache.org/r/72503/diff/2/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> HIVE-23449.2.patch
> https://reviews.apache.org/media/uploaded/files/2020/05/13/36724ea5-265e-4e61-98a4-18af913b5b48__HIVE-23449.2.patch
>
>
> Thanks,
>
> Rajesh Balamohan
>
>
Re: Review Request 72503: HIVE-23449: LLAP: Reduce mkdir and config
creations in submitWork hotpath
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72503/#review220734
-----------------------------------------------------------
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
Lines 197 (patched)
<https://reviews.apache.org/r/72503/#comment309406>
Aah I see, Make sense.
Can you add comment for this during commit in ContainerRunnerImpl.java for usage of supplier. As you can see, it wasnt apparent for me :)
- Ashutosh Chauhan
On May 13, 2020, 2:28 a.m., Rajesh Balamohan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72503/
> -----------------------------------------------------------
>
> (Updated May 13, 2020, 2:28 a.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> For short jobs, submitWork() becomes hotpath. Patch tries to lazy load conf creation and also gets rid of dir creations (which needs to be enabled only when DirWatcher is enabled in ShuffleHandler)
>
>
> Diffs
> -----
>
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java 6a13b55e69
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java eae8e08540
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java 36192520e3
> llap-server/src/java/org/apache/hadoop/hive/llap/shufflehandler/ShuffleHandler.java aff2c2ec39
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java 50dec4759e
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestContainerRunnerImpl.java 8ae00b9c87
>
>
> Diff: https://reviews.apache.org/r/72503/diff/2/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> HIVE-23449.2.patch
> https://reviews.apache.org/media/uploaded/files/2020/05/13/36724ea5-265e-4e61-98a4-18af913b5b48__HIVE-23449.2.patch
>
>
> Thanks,
>
> Rajesh Balamohan
>
>
Re: Review Request 72503: HIVE-23449: LLAP: Reduce mkdir and config
creations in submitWork hotpath
Posted by Rajesh Balamohan <rb...@apache.org>.
> On May 13, 2020, 2:45 a.m., Ashutosh Chauhan wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
> > Lines 197 (patched)
> > <https://reviews.apache.org/r/72503/diff/2/?file=2231479#file2231479line197>
> >
> > Still didn't follow. What is conf.get() saving here instead of passing constructed conf from caller?
The construction was the issue. With supplier, it is getting moved out of the IPC thread codepath.
- Rajesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72503/#review220732
-----------------------------------------------------------
On May 13, 2020, 2:28 a.m., Rajesh Balamohan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72503/
> -----------------------------------------------------------
>
> (Updated May 13, 2020, 2:28 a.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> For short jobs, submitWork() becomes hotpath. Patch tries to lazy load conf creation and also gets rid of dir creations (which needs to be enabled only when DirWatcher is enabled in ShuffleHandler)
>
>
> Diffs
> -----
>
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java 6a13b55e69
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java eae8e08540
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java 36192520e3
> llap-server/src/java/org/apache/hadoop/hive/llap/shufflehandler/ShuffleHandler.java aff2c2ec39
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java 50dec4759e
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestContainerRunnerImpl.java 8ae00b9c87
>
>
> Diff: https://reviews.apache.org/r/72503/diff/2/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> HIVE-23449.2.patch
> https://reviews.apache.org/media/uploaded/files/2020/05/13/36724ea5-265e-4e61-98a4-18af913b5b48__HIVE-23449.2.patch
>
>
> Thanks,
>
> Rajesh Balamohan
>
>
Re: Review Request 72503: HIVE-23449: LLAP: Reduce mkdir and config
creations in submitWork hotpath
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72503/#review220732
-----------------------------------------------------------
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
Lines 197 (patched)
<https://reviews.apache.org/r/72503/#comment309404>
Still didn't follow. What is conf.get() saving here instead of passing constructed conf from caller?
- Ashutosh Chauhan
On May 13, 2020, 2:28 a.m., Rajesh Balamohan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72503/
> -----------------------------------------------------------
>
> (Updated May 13, 2020, 2:28 a.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> For short jobs, submitWork() becomes hotpath. Patch tries to lazy load conf creation and also gets rid of dir creations (which needs to be enabled only when DirWatcher is enabled in ShuffleHandler)
>
>
> Diffs
> -----
>
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java 6a13b55e69
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java eae8e08540
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java 36192520e3
> llap-server/src/java/org/apache/hadoop/hive/llap/shufflehandler/ShuffleHandler.java aff2c2ec39
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java 50dec4759e
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestContainerRunnerImpl.java 8ae00b9c87
>
>
> Diff: https://reviews.apache.org/r/72503/diff/2/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> HIVE-23449.2.patch
> https://reviews.apache.org/media/uploaded/files/2020/05/13/36724ea5-265e-4e61-98a4-18af913b5b48__HIVE-23449.2.patch
>
>
> Thanks,
>
> Rajesh Balamohan
>
>
Re: Review Request 72503: HIVE-23449: LLAP: Reduce mkdir and config
creations in submitWork hotpath
Posted by Rajesh Balamohan <rb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72503/
-----------------------------------------------------------
(Updated May 13, 2020, 2:28 a.m.)
Review request for hive and Ashutosh Chauhan.
Changes
-------
Addressed review comments and fixed TestContainerRunnerImpl
Repository: hive-git
Description
-------
For short jobs, submitWork() becomes hotpath. Patch tries to lazy load conf creation and also gets rid of dir creations (which needs to be enabled only when DirWatcher is enabled in ShuffleHandler)
Diffs (updated)
-----
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java 6a13b55e69
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java eae8e08540
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java 36192520e3
llap-server/src/java/org/apache/hadoop/hive/llap/shufflehandler/ShuffleHandler.java aff2c2ec39
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java 50dec4759e
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestContainerRunnerImpl.java 8ae00b9c87
Diff: https://reviews.apache.org/r/72503/diff/2/
Changes: https://reviews.apache.org/r/72503/diff/1-2/
Testing
-------
File Attachments (updated)
----------------
HIVE-23449.2.patch
https://reviews.apache.org/media/uploaded/files/2020/05/13/36724ea5-265e-4e61-98a4-18af913b5b48__HIVE-23449.2.patch
Thanks,
Rajesh Balamohan
Re: Review Request 72503: HIVE-23449: LLAP: Reduce mkdir and config
creations in submitWork hotpath
Posted by Rajesh Balamohan <rb...@apache.org>.
> On May 13, 2020, 12:40 a.m., Ashutosh Chauhan wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
> > Line 257 (original), 259 (patched)
> > <https://reviews.apache.org/r/72503/diff/1/?file=2231471#file2231471line259>
> >
> > I am not sure how memoization helps here. This conf.get() will always be called for this task execution. So, we might as well pass in conf here. Is there something we are saving by memoization?
This is to avoid re-evaluation in callInternal twice. But ideally, it could be avoided by getting its reference once in callInternal. Will revise it in next patch.
- Rajesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72503/#review220729
-----------------------------------------------------------
On May 12, 2020, 9:59 p.m., Rajesh Balamohan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72503/
> -----------------------------------------------------------
>
> (Updated May 12, 2020, 9:59 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> For short jobs, submitWork() becomes hotpath. Patch tries to lazy load conf creation and also gets rid of dir creations (which needs to be enabled only when DirWatcher is enabled in ShuffleHandler)
>
>
> Diffs
> -----
>
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java 6a13b55e69
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java eae8e08540
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java 36192520e3
> llap-server/src/java/org/apache/hadoop/hive/llap/shufflehandler/ShuffleHandler.java aff2c2ec39
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java 50dec4759e
>
>
> Diff: https://reviews.apache.org/r/72503/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Rajesh Balamohan
>
>
Re: Review Request 72503: HIVE-23449: LLAP: Reduce mkdir and config
creations in submitWork hotpath
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72503/#review220729
-----------------------------------------------------------
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
Line 257 (original), 259 (patched)
<https://reviews.apache.org/r/72503/#comment309402>
I am not sure how memoization helps here. This conf.get() will always be called for this task execution. So, we might as well pass in conf here. Is there something we are saving by memoization?
- Ashutosh Chauhan
On May 12, 2020, 9:59 p.m., Rajesh Balamohan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72503/
> -----------------------------------------------------------
>
> (Updated May 12, 2020, 9:59 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> For short jobs, submitWork() becomes hotpath. Patch tries to lazy load conf creation and also gets rid of dir creations (which needs to be enabled only when DirWatcher is enabled in ShuffleHandler)
>
>
> Diffs
> -----
>
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java 6a13b55e69
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java eae8e08540
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java 36192520e3
> llap-server/src/java/org/apache/hadoop/hive/llap/shufflehandler/ShuffleHandler.java aff2c2ec39
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java 50dec4759e
>
>
> Diff: https://reviews.apache.org/r/72503/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Rajesh Balamohan
>
>