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:10:51 UTC
Review Request 36208: DRILL-3455: If a drillbit,
that contains fragments for the current query,
dies the QueryManager will fail the query even if those fragments already
finished successfully
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36208/
-----------------------------------------------------------
Review request for drill, abdelhakim deneche and Jacques Nadeau.
Bugs: DRILL-3455
https://issues.apache.org/jira/browse/DRILL-3455
Repository: drill-git
Description
-------
DRILL-3455: If fragments on unregistered Drillbits finished successfully, do not fail the query
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 9318233
Diff: https://reviews.apache.org/r/36208/diff/
Testing
-------
will update
Thanks,
Sudheesh Katkam
Re: Review Request 36208: DRILL-3455: If a drillbit,
that contains fragments for the current query,
dies the QueryManager will fail the query even if those fragments already
finished successfully
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36208/#review90518
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java (line 431)
<https://reviews.apache.org/r/36208/#comment143636>
It may be just a personal opinion, and you may safely ignore this comment, but shouldn't nodeDead() return true when it does it's job (marking running fragments as completed) and false otherwise ?
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java (line 518)
<https://reviews.apache.org/r/36208/#comment143637>
alreadyCompleted is not used anywhere, you should just call #nodeDead() inside the if
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java (line 534)
<https://reviews.apache.org/r/36208/#comment143638>
you should probably include your fix for DRILL-3448 as part of this patch, it's 3 lines change
- abdelhakim deneche
On July 6, 2015, 5:10 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36208/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 5:10 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Jacques Nadeau.
>
>
> Bugs: DRILL-3455
> https://issues.apache.org/jira/browse/DRILL-3455
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3455: If fragments on unregistered Drillbits finished successfully, do not fail the query
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 9318233
>
> Diff: https://reviews.apache.org/r/36208/diff/
>
>
> Testing
> -------
>
> will update
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 36208: DRILL-3455: If a drillbit,
that contains fragments for the current query,
dies the QueryManager will fail the query even if those fragments already
finished successfully
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36208/
-----------------------------------------------------------
(Updated July 23, 2015, 12:18 a.m.)
Review request for drill, abdelhakim deneche and Jacques Nadeau.
Changes
-------
Addressed Jacques' comments
Bugs: DRILL-3455
https://issues.apache.org/jira/browse/DRILL-3455
Repository: drill-git
Description
-------
DRILL-3455: If fragments on unregistered Drillbits finished successfully, do not fail the query
+ DRILL-3448: Flipped the atLeastOneFailure condition in QueryManager
+ fixes in DrillbitStatusListener interface
+ logs from implementations of DrillbitStatusListener
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/DrillbitStatusListener.java 80f2ca1
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 9318233
Diff: https://reviews.apache.org/r/36208/diff/
Testing
-------
passes unit tests
Thanks,
Sudheesh Katkam
Re: Review Request 36208: DRILL-3455: If a drillbit,
that contains fragments for the current query,
dies the QueryManager will fail the query even if those fragments already
finished successfully
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36208/#review91657
-----------------------------------------------------------
Ship it!
Looks good. A small improvement on the javadoc might be good.
+1
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java (line 428)
<https://reviews.apache.org/r/36208/#comment145180>
This comment is a bit confusing. Maybe something similar to:
Returns true if the node has fragments that are pending (non-terminal state). Returns false if all fragments running on this node have already terminated.
- Jacques Nadeau
On July 6, 2015, 6:47 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36208/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 6:47 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Jacques Nadeau.
>
>
> Bugs: DRILL-3455
> https://issues.apache.org/jira/browse/DRILL-3455
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3455: If fragments on unregistered Drillbits finished successfully, do not fail the query
>
> + DRILL-3448: Flipped the atLeastOneFailure condition in QueryManager
> + fixes in DrillbitStatusListener interface
> + logs from implementations of DrillbitStatusListener
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/DrillbitStatusListener.java 80f2ca1
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 9318233
>
> Diff: https://reviews.apache.org/r/36208/diff/
>
>
> Testing
> -------
>
> passes unit tests
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 36208: DRILL-3455: If a drillbit,
that contains fragments for the current query,
dies the QueryManager will fail the query even if those fragments already
finished successfully
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36208/#review90541
-----------------------------------------------------------
+1 (non-binding)
- abdelhakim deneche
On July 6, 2015, 6:47 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36208/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 6:47 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Jacques Nadeau.
>
>
> Bugs: DRILL-3455
> https://issues.apache.org/jira/browse/DRILL-3455
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3455: If fragments on unregistered Drillbits finished successfully, do not fail the query
>
> + DRILL-3448: Flipped the atLeastOneFailure condition in QueryManager
> + fixes in DrillbitStatusListener interface
> + logs from implementations of DrillbitStatusListener
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/DrillbitStatusListener.java 80f2ca1
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 9318233
>
> Diff: https://reviews.apache.org/r/36208/diff/
>
>
> Testing
> -------
>
> passes unit tests
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 36208: DRILL-3455: If a drillbit,
that contains fragments for the current query,
dies the QueryManager will fail the query even if those fragments already
finished successfully
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36208/
-----------------------------------------------------------
(Updated July 6, 2015, 6:47 p.m.)
Review request for drill, abdelhakim deneche and Jacques Nadeau.
Changes
-------
Addressed Hakkim's comments
Bugs: DRILL-3455
https://issues.apache.org/jira/browse/DRILL-3455
Repository: drill-git
Description (updated)
-------
DRILL-3455: If fragments on unregistered Drillbits finished successfully, do not fail the query
+ DRILL-3448: Flipped the atLeastOneFailure condition in QueryManager
+ fixes in DrillbitStatusListener interface
+ logs from implementations of DrillbitStatusListener
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/DrillbitStatusListener.java 80f2ca1
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 9318233
Diff: https://reviews.apache.org/r/36208/diff/
Testing (updated)
-------
passes unit tests
Thanks,
Sudheesh Katkam