You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Sudheesh Katkam <sk...@maprtech.com> on 2015/07/06 19:15:21 UTC
Review Request 36210: DRILL-3341: Move OperatorWrapper list and
FragmentWrapper list creation to ProfileWrapper ctor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36210/
-----------------------------------------------------------
Review request for drill and Jason Altekruse.
Bugs: DRILL-3341
https://issues.apache.org/jira/browse/DRILL-3341
Repository: drill-git
Description
-------
DRILL-3341: Avoid recomputing stats in ProfileWrapper
+ Small optimizations and comments in OperatorWrapper and FragmentWrapper
+ Comparator renames for consistency
+ Move TableBuilder#toString logic to TableBuilder#build
+ Incorrect field names in profile page for failed queries
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/Comparators.java 20f76a2
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java e395e3e
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java 403a23f
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileWrapper.java dd26a76
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/TableBuilder.java 8616820
exec/java-exec/src/main/resources/rest/profile/profile.ftl 46cdc83
Diff: https://reviews.apache.org/r/36210/diff/
Testing
-------
will update
Thanks,
Sudheesh Katkam
Re: Review Request 36210: DRILL-3341: Move OperatorWrapper list and
FragmentWrapper list creation to ProfileWrapper ctor
Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36210/#review92328
-----------------------------------------------------------
Ship it!
Ship It!
- Jason Altekruse
On July 13, 2015, 11:08 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36210/
> -----------------------------------------------------------
>
> (Updated July 13, 2015, 11:08 p.m.)
>
>
> Review request for drill and Jason Altekruse.
>
>
> Bugs: DRILL-3341
> https://issues.apache.org/jira/browse/DRILL-3341
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3341: Avoid recomputing stats in ProfileWrapper
> + Small optimizations and comments in OperatorWrapper and FragmentWrapper
> + Comparator renames for consistency
> + Move TableBuilder#toString logic to TableBuilder#build
> + Incorrect field names in profile page for failed queries
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/Comparators.java 20f76a2
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java e395e3e
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java 403a23f
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileWrapper.java dd26a76
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/TableBuilder.java 8616820
> exec/java-exec/src/main/resources/rest/profile/profile.ftl 46cdc83
>
> Diff: https://reviews.apache.org/r/36210/diff/
>
>
> Testing
> -------
>
> will update
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 36210: DRILL-3341: Move OperatorWrapper list and
FragmentWrapper list creation to ProfileWrapper ctor
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36210/
-----------------------------------------------------------
(Updated July 13, 2015, 11:08 p.m.)
Review request for drill and Jason Altekruse.
Changes
-------
Addressing review comments.
Bugs: DRILL-3341
https://issues.apache.org/jira/browse/DRILL-3341
Repository: drill-git
Description
-------
DRILL-3341: Avoid recomputing stats in ProfileWrapper
+ Small optimizations and comments in OperatorWrapper and FragmentWrapper
+ Comparator renames for consistency
+ Move TableBuilder#toString logic to TableBuilder#build
+ Incorrect field names in profile page for failed queries
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/Comparators.java 20f76a2
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java e395e3e
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java 403a23f
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileWrapper.java dd26a76
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/TableBuilder.java 8616820
exec/java-exec/src/main/resources/rest/profile/profile.ftl 46cdc83
Diff: https://reviews.apache.org/r/36210/diff/
Testing
-------
will update
Thanks,
Sudheesh Katkam
Re: Review Request 36210: DRILL-3341: Move OperatorWrapper list and
FragmentWrapper list creation to ProfileWrapper ctor
Posted by Sudheesh Katkam <sk...@maprtech.com>.
> On July 9, 2015, 2:59 p.m., Jason Altekruse wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java, line 105
> > <https://reviews.apache.org/r/36210/diff/1/?file=1000325#file1000325line105>
> >
> > Can we just change the name?
Profiles don't have versions. This could break UI when reading profiles (see DRILL-3494).
- Sudheesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36210/#review90544
-----------------------------------------------------------
On July 6, 2015, 5:15 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36210/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 5:15 p.m.)
>
>
> Review request for drill and Jason Altekruse.
>
>
> Bugs: DRILL-3341
> https://issues.apache.org/jira/browse/DRILL-3341
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3341: Avoid recomputing stats in ProfileWrapper
> + Small optimizations and comments in OperatorWrapper and FragmentWrapper
> + Comparator renames for consistency
> + Move TableBuilder#toString logic to TableBuilder#build
> + Incorrect field names in profile page for failed queries
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/Comparators.java 20f76a2
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java e395e3e
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java 403a23f
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileWrapper.java dd26a76
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/TableBuilder.java 8616820
> exec/java-exec/src/main/resources/rest/profile/profile.ftl 46cdc83
>
> Diff: https://reviews.apache.org/r/36210/diff/
>
>
> Testing
> -------
>
> will update
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 36210: DRILL-3341: Move OperatorWrapper list and
FragmentWrapper list creation to ProfileWrapper ctor
Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36210/#review90544
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/Comparators.java (line 68)
<https://reviews.apache.org/r/36210/#comment143654>
Making these consistent is good, but I think the new names are confusing. I would make these end in comparator or just remove the suffix. They are declared withint an interface called Comparators which you have added a resonable comment to describe them all.
Looking where these are referenced below, they aren't using static imports, so we end up writing Comparators.fragPeakMemoryCompare, I think you can just change it so we reference them as Comparators.fragPeakMemory
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java (line 103)
<https://reviews.apache.org/r/36210/#comment144207>
Can we just change the name?
- Jason Altekruse
On July 6, 2015, 5:15 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36210/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 5:15 p.m.)
>
>
> Review request for drill and Jason Altekruse.
>
>
> Bugs: DRILL-3341
> https://issues.apache.org/jira/browse/DRILL-3341
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3341: Avoid recomputing stats in ProfileWrapper
> + Small optimizations and comments in OperatorWrapper and FragmentWrapper
> + Comparator renames for consistency
> + Move TableBuilder#toString logic to TableBuilder#build
> + Incorrect field names in profile page for failed queries
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/Comparators.java 20f76a2
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java e395e3e
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java 403a23f
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileWrapper.java dd26a76
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/TableBuilder.java 8616820
> exec/java-exec/src/main/resources/rest/profile/profile.ftl 46cdc83
>
> Diff: https://reviews.apache.org/r/36210/diff/
>
>
> Testing
> -------
>
> will update
>
>
> Thanks,
>
> Sudheesh Katkam
>
>