You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Jacques Nadeau <ja...@gmail.com> on 2015/04/13 02:07:28 UTC
Review Request 33115: DRILL-2762: Update Fragment state reporting and
error collection
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/
-----------------------------------------------------------
Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
Repository: drill-git
Description
-------
DRILL-2762: Update Fragment state reporting and error collection
DeferredException
- Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
BufferManager
- Ensure close() can be called multiple times by clearing managed buffer list on close().
FragmentContext/FragmentExecutor
- Update FragmentContext to have a preClose so that we can check closure state before doing final close.
- Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
WorkManager Updates
- Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
- Change status thread so it only reports non-terminal status to the Foreman to avoid race conditions and confusion when receiving multiple possibly incomplete terminal messages.
Foreman/QueryManager
- Extract listenable interfaces into anonymous inner classes from body of Foreman
QueryManager
- Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
- Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
FragmentData/MinorFragmentProfile
- Add ability to track last status update as well as last time fragment made progress
AbstractRecordBatch
- Update awareness of current cancellation state to avoid cancellation delays
Diffs
-----
common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java ac1bcbb
protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
protocol/src/main/protobuf/UserBitShared.proto 2938114
Diff: https://reviews.apache.org/r/33115/diff/
Testing
-------
Regression & Unit, more manual testing planned before final patch.
Thanks,
Jacques Nadeau
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/#review79916
-----------------------------------------------------------
What's driving your desire for the NodeTracker? Do you really think it's likely that we'll get notifications for multiple fragment completions from a single remote node at the same time, and that this is a frequent occurrence? I guess it could happen with non-blocking operators like filters, but are the notifications set up to be grouped in order to be able to take advantage of this alternate data structure layout?
common/src/main/java/org/apache/drill/common/DeferredException.java
<https://reviews.apache.org/r/33115/#comment129510>
Space between Exception and opening brace.
With this, I would now replace the body of the try block inside close() with a call to throwAndClear(). (Then the only difference will be that close() also sets isClosed().
Since my other comment suggests checking isClosed here, then that check can be removed from close(), but keep the setting there.
common/src/main/java/org/apache/drill/common/DeferredException.java
<https://reviews.apache.org/r/33115/#comment129511>
No "this." required here; space between closing paren and opening brace. Also, I think you should throw IllegalStateException if isClosed.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/33115/#comment129513>
Space before opening brace; are your IDE template selections set up right?
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java
<https://reviews.apache.org/r/33115/#comment129514>
Space before opening brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java
<https://reviews.apache.org/r/33115/#comment129516>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java
<https://reviews.apache.org/r/33115/#comment129524>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java
<https://reviews.apache.org/r/33115/#comment129529>
We should capture dContext.getController() into a final local before the while loop.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129540>
Before this line
final FragmentState currentState = getState();
then use currentState on this line, and below on line 66, where you repeat this m.f().f().
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129538>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129539>
Finish comment?
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129541>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129533>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129534>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129537>
Please no empty comments at the ends of fluent-style lines like this. In this particular case, line formatting should be
return status.getProfile().toBuilder()
.setLastUpdate(lastStatusUpdate)
.setLastProgress(lastProgress)
.build()
The builder itself gets a line per attribute setting, but not the other calls, which aren't actually building anything. build() gets it's own end-of-list line.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129542>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129543>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129544>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129545>
Space after assign and before zero. Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129546>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129548>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment129547>
No empty comments on ends of continued statement lines.
If you want to follow Google coding style (I don't care that strongly, but that's what seems to be the case elsewhere), the || go at the beginning of the next line (like the dots in fluent-style invocations).
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129549>
Woohoo, those have been a thorn in my side for a while...
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129550>
final
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129551>
final
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129552>
Space before open brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129553>
final
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129555>
How do you know it's complete just because the state changed?
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129554>
No this here.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129558>
Space before brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129566>
Space before brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129567>
Space before brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129613>
The contents of this if block feel like they don't belong here because the manipulate the global state, rather than just the state of this node. This should be moved to a nodeComplete() method that's directly on QueryManager.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129623>
finishedFragments is not the number of fragments we're waiting for. With this block moved up to QueryManager.nodeComplete(), it doesn't make sense to keep that second value in the string.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129579>
Finish comment?
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129581>
Space before brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129582>
Space before brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129580>
No blank line here.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129583>
Let's rename this to newRootStatusHandler().
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129585>
Space before brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129587>
Space before brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129594>
final
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129588>
final
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129610>
Why add these to a list, and then walk through that list? Why not just take the block below, and put it here instead?
Afterwards, you can check the StringBuilder's size to see if you need to write out the warning and call moveToState. Or, if you don't like that, set a boolean flag.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129590>
Space before brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129606>
When you don't have to worry about thread safety, as here, use StringBuilder instead. Also, final.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129591>
Space before brace.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129595>
How about adding a colon here inbetween?
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33115/#comment129615>
"so" => "because"?
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33115/#comment129616>
Do we need the canceled member anymore? In order to avoid problems with synchronization of duplicate state, why not replace it with calls to fragmentContext.isCancelled(), (or shouldContinue()) as appropriate?
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33115/#comment129617>
fragmentContext.shouldContinue() instead of !canceled?
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33115/#comment129618>
Isn't the goal here to make sure we catch and report all exceptions except for any that might occur during the final response to the client (because by then it's too late, since we've already formatted the final message to the client)? We should make that explicit here so it's clear why we have this two-phase closing.
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33115/#comment129619>
No blank line here.
- Chris Westin
On April 12, 2015, 5:07 p.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33115/
> -----------------------------------------------------------
>
> (Updated April 12, 2015, 5:07 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2762: Update Fragment state reporting and error collection
>
> DeferredException
> - Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
>
> BufferManager
> - Ensure close() can be called multiple times by clearing managed buffer list on close().
>
> FragmentContext/FragmentExecutor
> - Update FragmentContext to have a preClose so that we can check closure state before doing final close.
> - Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
>
> WorkManager Updates
> - Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
> - Change status thread so it only reports non-terminal status to the Foreman to avoid race conditions and confusion when receiving multiple possibly incomplete terminal messages.
>
> Foreman/QueryManager
> - Extract listenable interfaces into anonymous inner classes from body of Foreman
>
> QueryManager
> - Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
> - Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
>
> FragmentData/MinorFragmentProfile
> - Add ability to track last status update as well as last time fragment made progress
>
> AbstractRecordBatch
> - Update awareness of current cancellation state to avoid cancellation delays
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
> exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
> protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
> protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java ac1bcbb
> protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
> protocol/src/main/protobuf/UserBitShared.proto 2938114
>
> Diff: https://reviews.apache.org/r/33115/diff/
>
>
> Testing
> -------
>
> Regression & Unit, more manual testing planned before final patch.
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/#review79987
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129709>
finishedNodes <= finishedNodes
shouldn't this be:
finishedNodes <= totalNodes ?
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment129710>
unnecessary semicolon
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33115/#comment129707>
unused import
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33115/#comment129706>
updateState() is not used anywhere, should we just remove it ?
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33115/#comment129708>
iff -> if
- abdelhakim deneche
On April 13, 2015, 12:07 a.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33115/
> -----------------------------------------------------------
>
> (Updated April 13, 2015, 12:07 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2762: Update Fragment state reporting and error collection
>
> DeferredException
> - Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
>
> BufferManager
> - Ensure close() can be called multiple times by clearing managed buffer list on close().
>
> FragmentContext/FragmentExecutor
> - Update FragmentContext to have a preClose so that we can check closure state before doing final close.
> - Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
>
> WorkManager Updates
> - Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
> - Change status thread so it only reports non-terminal status to the Foreman to avoid race conditions and confusion when receiving multiple possibly incomplete terminal messages.
>
> Foreman/QueryManager
> - Extract listenable interfaces into anonymous inner classes from body of Foreman
>
> QueryManager
> - Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
> - Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
>
> FragmentData/MinorFragmentProfile
> - Add ability to track last status update as well as last time fragment made progress
>
> AbstractRecordBatch
> - Update awareness of current cancellation state to avoid cancellation delays
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
> exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
> protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
> protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java ac1bcbb
> protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
> protocol/src/main/protobuf/UserBitShared.proto 2938114
>
> Diff: https://reviews.apache.org/r/33115/diff/
>
>
> Testing
> -------
>
> Regression & Unit, more manual testing planned before final patch.
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/#review80216
-----------------------------------------------------------
I am running with this patch on the performance cluster and noticed that the query profile display is all messed up - probably because of new fields added to the profile ? Some operators are missing on the display and the ones that are shown have incorrect fields.
- Aman Sinha
On April 15, 2015, 5:22 a.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33115/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 5:22 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2762: Update Fragment state reporting and error collection
>
> DeferredException
> - Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
> - Add new getAndClear operation
>
> BufferManager
> - Ensure close() can be called multiple times by clearing managed buffer list on close().
>
> FragmentContext/FragmentExecutor
> - Update FragmentContext to have a preClose so that we can check closure state before doing final close.
> - Update so that there is only a single state maintained between FragmentContext and FragmentExecutor
> - Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
> - Add new CANCELLATION_REQUESTED state for FragmentState.
> - Move all users of isCancelled or isFailed in main code to use shouldContinue()
> - Update receivingFragmentFinished message to not cancel fragment (only inform root operator of cancellation)
>
> WorkManager Updates
> - Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
>
> Foreman/QueryManager
> - Extract listenable interfaces into anonymous inner classes from body of Foreman
>
> QueryManager
> - Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
> - Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
>
> FragmentData/MinorFragmentProfile
> - Add ability to track last status update as well as last time fragment made progress
>
> AbstractRecordBatch
> - Update awareness of current cancellation state to avoid cancellation delays
>
> Misc. Other changes
> - Move ByteCode optimization code to only record assembly and code as trace messages
> - Update SimpleRootExec to create fake ExecutorState to make existing tests work.
> - Update sort to exit prematurely in the case that the fragment was asked to cancel.
> - Add finals to all edited files.
> - Modify control handler and FragmentManager to directly support receivingFragmentFinished
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
> exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
> protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
> protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java ac1bcbb
> protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
> protocol/src/main/protobuf/UserBitShared.proto 2938114
>
> Diff: https://reviews.apache.org/r/33115/diff/
>
>
> Testing
> -------
>
> Regression & Unit, more manual testing planned before final patch.
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/#review80388
-----------------------------------------------------------
common/src/main/java/org/apache/drill/common/DeferredException.java
<https://reviews.apache.org/r/33115/#comment130280>
Is deferred exception closed at all ? I can't seem to find where this is closed
- abdelhakim deneche
On April 15, 2015, 7 p.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33115/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 7 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2762: Update Fragment state reporting and error collection
>
> DeferredException
> - Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
> - Add new getAndClear operation
>
> BufferManager
> - Ensure close() can be called multiple times by clearing managed buffer list on close().
>
> FragmentContext/FragmentExecutor
> - Update FragmentContext to have a preClose so that we can check closure state before doing final close.
> - Update so that there is only a single state maintained between FragmentContext and FragmentExecutor
> - Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
> - Add new CANCELLATION_REQUESTED state for FragmentState.
> - Move all users of isCancelled or isFailed in main code to use shouldContinue()
> - Update receivingFragmentFinished message to not cancel fragment (only inform root operator of cancellation)
>
> WorkManager Updates
> - Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
>
> Foreman/QueryManager
> - Extract listenable interfaces into anonymous inner classes from body of Foreman
>
> QueryManager
> - Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
> - Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
>
> FragmentData/MinorFragmentProfile
> - Add ability to track last status update as well as last time fragment made progress
>
> AbstractRecordBatch
> - Update awareness of current cancellation state to avoid cancellation delays
>
> Misc. Other changes
> - Move ByteCode optimization code to only record assembly and code as trace messages
> - Update SimpleRootExec to create fake ExecutorState to make existing tests work.
> - Update sort to exit prematurely in the case that the fragment was asked to cancel.
> - Add finals to all edited files.
> - Modify control handler and FragmentManager to directly support receivingFragmentFinished
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/AsmUtil.java 81904df
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java 6c0292e
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java 6e981bc
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java 5b7ca66
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java e230fd2
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/producer/ProducerConsumerBatch.java c50cb8a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 4b317e0
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 389d668
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java a23780e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 94bc3a3
> exec/java-exec/src/main/java/org/apache/drill/exec/proto/helper/QueryIdHelper.java 1aadaa2
> exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
> exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java c15bb7c
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java 3a7123d
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/UnlimitedRawBatchBuffer.java 2430e64
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/AbstractStatusReporter.java 1b0885d
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 7a819c4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java 41e87cd
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 84071c3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StateTransitionException.java 7155d43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StatusReporter.java 26b5d68
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/SimpleRootExec.java 933417e
> protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
> protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 96a921b
> protocol/src/main/java/org/apache/drill/exec/proto/beans/FragmentState.java ba536fc
> protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
> protocol/src/main/protobuf/UserBitShared.proto 2938114
>
> Diff: https://reviews.apache.org/r/33115/diff/
>
>
> Testing
> -------
>
> Regression & Unit, more manual testing planned before final patch.
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by Jacques Nadeau <ja...@gmail.com>.
> On April 15, 2015, 8:24 p.m., Chris Westin wrote:
> > common/src/main/java/org/apache/drill/common/DeferredException.java, line 133
> > <https://reviews.apache.org/r/33115/diff/2-3/?file=929596#file929596line133>
> >
> > It's funny that you fixed your braces everywhere else, and then you added these, which were ok before.
Note sure what happened.
> On April 15, 2015, 8:24 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java, line 58
> > <https://reviews.apache.org/r/33115/diff/3/?file=930960#file930960line58>
> >
> > If eclipse added final to this, I'd check to see if it's even referenced.
It's referenced but never changed.
> On April 15, 2015, 8:24 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/proto/helper/QueryIdHelper.java, line 52
> > <https://reviews.apache.org/r/33115/diff/3/?file=930971#file930971line52>
> >
> > What will List<Integer>.toString() do here? I bet you only get the object identifier for the list.
AbstractCollection does the right thing.
- Jacques
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/#review80262
-----------------------------------------------------------
On April 15, 2015, 7 p.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33115/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 7 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2762: Update Fragment state reporting and error collection
>
> DeferredException
> - Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
> - Add new getAndClear operation
>
> BufferManager
> - Ensure close() can be called multiple times by clearing managed buffer list on close().
>
> FragmentContext/FragmentExecutor
> - Update FragmentContext to have a preClose so that we can check closure state before doing final close.
> - Update so that there is only a single state maintained between FragmentContext and FragmentExecutor
> - Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
> - Add new CANCELLATION_REQUESTED state for FragmentState.
> - Move all users of isCancelled or isFailed in main code to use shouldContinue()
> - Update receivingFragmentFinished message to not cancel fragment (only inform root operator of cancellation)
>
> WorkManager Updates
> - Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
>
> Foreman/QueryManager
> - Extract listenable interfaces into anonymous inner classes from body of Foreman
>
> QueryManager
> - Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
> - Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
>
> FragmentData/MinorFragmentProfile
> - Add ability to track last status update as well as last time fragment made progress
>
> AbstractRecordBatch
> - Update awareness of current cancellation state to avoid cancellation delays
>
> Misc. Other changes
> - Move ByteCode optimization code to only record assembly and code as trace messages
> - Update SimpleRootExec to create fake ExecutorState to make existing tests work.
> - Update sort to exit prematurely in the case that the fragment was asked to cancel.
> - Add finals to all edited files.
> - Modify control handler and FragmentManager to directly support receivingFragmentFinished
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/AsmUtil.java 81904df
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java 6c0292e
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java 6e981bc
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java 5b7ca66
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java e230fd2
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/producer/ProducerConsumerBatch.java c50cb8a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 4b317e0
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 389d668
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java a23780e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 94bc3a3
> exec/java-exec/src/main/java/org/apache/drill/exec/proto/helper/QueryIdHelper.java 1aadaa2
> exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
> exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java c15bb7c
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java 3a7123d
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/UnlimitedRawBatchBuffer.java 2430e64
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/AbstractStatusReporter.java 1b0885d
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 7a819c4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java 41e87cd
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 84071c3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StateTransitionException.java 7155d43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StatusReporter.java 26b5d68
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/SimpleRootExec.java 933417e
> protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
> protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 96a921b
> protocol/src/main/java/org/apache/drill/exec/proto/beans/FragmentState.java ba536fc
> protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
> protocol/src/main/protobuf/UserBitShared.proto 2938114
>
> Diff: https://reviews.apache.org/r/33115/diff/
>
>
> Testing
> -------
>
> Regression & Unit, more manual testing planned before final patch.
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/#review80262
-----------------------------------------------------------
common/src/main/java/org/apache/drill/common/DeferredException.java
<https://reviews.apache.org/r/33115/#comment130076>
It's funny that you fixed your braces everywhere else, and then you added these, which were ok before.
exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java
<https://reviews.apache.org/r/33115/#comment130069>
If eclipse added final to this, I'd check to see if it's even referenced.
exec/java-exec/src/main/java/org/apache/drill/exec/proto/helper/QueryIdHelper.java
<https://reviews.apache.org/r/33115/#comment130071>
What will List<Integer>.toString() do here? I bet you only get the object identifier for the list.
- Chris Westin
On April 15, 2015, noon, Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33115/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, noon)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2762: Update Fragment state reporting and error collection
>
> DeferredException
> - Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
> - Add new getAndClear operation
>
> BufferManager
> - Ensure close() can be called multiple times by clearing managed buffer list on close().
>
> FragmentContext/FragmentExecutor
> - Update FragmentContext to have a preClose so that we can check closure state before doing final close.
> - Update so that there is only a single state maintained between FragmentContext and FragmentExecutor
> - Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
> - Add new CANCELLATION_REQUESTED state for FragmentState.
> - Move all users of isCancelled or isFailed in main code to use shouldContinue()
> - Update receivingFragmentFinished message to not cancel fragment (only inform root operator of cancellation)
>
> WorkManager Updates
> - Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
>
> Foreman/QueryManager
> - Extract listenable interfaces into anonymous inner classes from body of Foreman
>
> QueryManager
> - Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
> - Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
>
> FragmentData/MinorFragmentProfile
> - Add ability to track last status update as well as last time fragment made progress
>
> AbstractRecordBatch
> - Update awareness of current cancellation state to avoid cancellation delays
>
> Misc. Other changes
> - Move ByteCode optimization code to only record assembly and code as trace messages
> - Update SimpleRootExec to create fake ExecutorState to make existing tests work.
> - Update sort to exit prematurely in the case that the fragment was asked to cancel.
> - Add finals to all edited files.
> - Modify control handler and FragmentManager to directly support receivingFragmentFinished
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/AsmUtil.java 81904df
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java 6c0292e
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java 6e981bc
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java 5b7ca66
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java e230fd2
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/producer/ProducerConsumerBatch.java c50cb8a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 4b317e0
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 389d668
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java a23780e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 94bc3a3
> exec/java-exec/src/main/java/org/apache/drill/exec/proto/helper/QueryIdHelper.java 1aadaa2
> exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
> exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java c15bb7c
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java 3a7123d
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/UnlimitedRawBatchBuffer.java 2430e64
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/AbstractStatusReporter.java 1b0885d
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 7a819c4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java 41e87cd
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 84071c3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StateTransitionException.java 7155d43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StatusReporter.java 26b5d68
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/SimpleRootExec.java 933417e
> protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
> protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 96a921b
> protocol/src/main/java/org/apache/drill/exec/proto/beans/FragmentState.java ba536fc
> protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
> protocol/src/main/protobuf/UserBitShared.proto 2938114
>
> Diff: https://reviews.apache.org/r/33115/diff/
>
>
> Testing
> -------
>
> Regression & Unit, more manual testing planned before final patch.
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/#review80331
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java
<https://reviews.apache.org/r/33115/#comment130181>
And there's only one reference, in a ternary conditional -- we should get rid of it, and just use the false branch of the conditional.
- Chris Westin
On April 15, 2015, noon, Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33115/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, noon)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2762: Update Fragment state reporting and error collection
>
> DeferredException
> - Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
> - Add new getAndClear operation
>
> BufferManager
> - Ensure close() can be called multiple times by clearing managed buffer list on close().
>
> FragmentContext/FragmentExecutor
> - Update FragmentContext to have a preClose so that we can check closure state before doing final close.
> - Update so that there is only a single state maintained between FragmentContext and FragmentExecutor
> - Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
> - Add new CANCELLATION_REQUESTED state for FragmentState.
> - Move all users of isCancelled or isFailed in main code to use shouldContinue()
> - Update receivingFragmentFinished message to not cancel fragment (only inform root operator of cancellation)
>
> WorkManager Updates
> - Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
>
> Foreman/QueryManager
> - Extract listenable interfaces into anonymous inner classes from body of Foreman
>
> QueryManager
> - Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
> - Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
>
> FragmentData/MinorFragmentProfile
> - Add ability to track last status update as well as last time fragment made progress
>
> AbstractRecordBatch
> - Update awareness of current cancellation state to avoid cancellation delays
>
> Misc. Other changes
> - Move ByteCode optimization code to only record assembly and code as trace messages
> - Update SimpleRootExec to create fake ExecutorState to make existing tests work.
> - Update sort to exit prematurely in the case that the fragment was asked to cancel.
> - Add finals to all edited files.
> - Modify control handler and FragmentManager to directly support receivingFragmentFinished
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/AsmUtil.java 81904df
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java 6c0292e
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java 6e981bc
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java 5b7ca66
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java e230fd2
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/producer/ProducerConsumerBatch.java c50cb8a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 4b317e0
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 389d668
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java a23780e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 94bc3a3
> exec/java-exec/src/main/java/org/apache/drill/exec/proto/helper/QueryIdHelper.java 1aadaa2
> exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
> exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java c15bb7c
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java 3a7123d
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/UnlimitedRawBatchBuffer.java 2430e64
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/AbstractStatusReporter.java 1b0885d
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 7a819c4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java 41e87cd
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 84071c3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StateTransitionException.java 7155d43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StatusReporter.java 26b5d68
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/SimpleRootExec.java 933417e
> protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
> protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 96a921b
> protocol/src/main/java/org/apache/drill/exec/proto/beans/FragmentState.java ba536fc
> protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
> protocol/src/main/protobuf/UserBitShared.proto 2938114
>
> Diff: https://reviews.apache.org/r/33115/diff/
>
>
> Testing
> -------
>
> Regression & Unit, more manual testing planned before final patch.
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/#review80478
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
<https://reviews.apache.org/r/33115/#comment130387>
What happened to all the finals in this file?
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
<https://reviews.apache.org/r/33115/#comment130386>
We shouldn't be making filenames in this way, but if we must, at least use File.pathSeparator instead of "/".
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment130436>
Now this no longer needs the "this."
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment130437>
no "this."
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/33115/#comment130438>
No "this."
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/33115/#comment130440>
Hadn't you better check rootRunner for null *before* you dereference it?
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/AbstractStatusReporter.java
<https://reviews.apache.org/r/33115/#comment130441>
Javadoc for this overridable? (And for the other protected overridables?)
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33115/#comment130443>
No "this." required on this one.
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33115/#comment130450>
This comment doesn't seem right. countDown() doesn't do anything to prevent this thread exiting. However, it will prevent any other threads who are waiting to deliver messages (via cancel() et al) from being blocked forever (an issue that was making things hang around Foreman when there were early errors during setup). => this comment needs some work.
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java
<https://reviews.apache.org/r/33115/#comment130465>
Since we're no longer calling new each time, why does this matter anymore? getRunnable() can just be "return cancel ? null : runner;"
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java
<https://reviews.apache.org/r/33115/#comment130468>
From what I can see here without the benefit of an IDE, it looks like runner is never NULL; it's final, and it's initialized in the constructor, so all this conditional stuff is dead code that can go.
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java
<https://reviews.apache.org/r/33115/#comment130469>
As per my comment in receivingFragmentFinished(), this can never happen, so get rid of the conditional.
common/src/main/java/org/apache/drill/common/concurrent/ExtendedLatch.java
<https://reviews.apache.org/r/33115/#comment130380>
In this isolated context, can we skip the local variable altogether, and just use a while(true) break (or return) where you currently set ready=true?
common/src/main/java/org/apache/drill/common/exceptions/UserException.java
<https://reviews.apache.org/r/33115/#comment130381>
Don't need a "this." here.
exec/java-exec/src/main/java/org/apache/drill/exec/record/FragmentWritableBatch.java
<https://reviews.apache.org/r/33115/#comment130389>
newBuilder() goes on the previous line.
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java
<https://reviews.apache.org/r/33115/#comment130421>
This comment is for this entire function.
Instead of repeatedly sorting "complete" to get the minimum or maximum of certain values, instead use http://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#min(java.util.Collection,%20java.util.Comparator) and http://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#max(java.util.Collection,%20java.util.Comparator) . => instead of a series of nlog(n) ops, we get a series of O(n) ops.
After this substitution, you can use a LinkedList<> instead of an ArrayList<> for "complete," as that avoids repeated reallocation (and eventual oversizing) as things are added to the list, and eliminate the now obsolete variable "li."
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/TableBuilder.java
<https://reviews.apache.org/r/33115/#comment130432>
Shouldn't these be one or more of private, static, and final?
- Chris Westin
On April 17, 2015, 10:04 a.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33115/
> -----------------------------------------------------------
>
> (Updated April 17, 2015, 10:04 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2762: Update Fragment state reporting and error collection
>
> DeferredException
> - Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
> - Add new getAndClear operation
>
> BufferManager
> - Ensure close() can be called multiple times by clearing managed buffer list on close().
>
> FragmentContext/FragmentExecutor
> - Update FragmentContext to have a preClose so that we can check closure state before doing final close.
> - Update so that there is only a single state maintained between FragmentContext and FragmentExecutor
> - Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
> - Add new CANCELLATION_REQUESTED state for FragmentState.
> - Move all users of isCancelled or isFailed in main code to use shouldContinue()
> - Update receivingFragmentFinished message to not cancel fragment (only inform root operator of cancellation)
>
> WorkManager Updates
> - Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
>
> Foreman/QueryManager
> - Extract listenable interfaces into anonymous inner classes from body of Foreman
>
> QueryManager
> - Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
> - Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
>
> FragmentData/MinorFragmentProfile
> - Add ability to track last status update as well as last time fragment made progress
>
> AbstractRecordBatch
> - Update awareness of current cancellation state to avoid cancellation delays
>
> Misc. Other changes
> - Move ByteCode optimization code to only record assembly and code as trace messages
> - Update SimpleRootExec to create fake ExecutorState to make existing tests work.
> - Update sort to exit prematurely in the case that the fragment was asked to cancel.
> - Add finals to all edited files.
> - Modify control handler and FragmentManager to directly support receivingFragmentFinished
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
> common/src/main/java/org/apache/drill/common/concurrent/ExtendedLatch.java PRE-CREATION
> common/src/main/java/org/apache/drill/common/exceptions/UserException.java e995346
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/AsmUtil.java 81904df
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java 6c0292e
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java 6e981bc
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 44ca78a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java 5b7ca66
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java e230fd2
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/producer/ProducerConsumerBatch.java c50cb8a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 4b317e0
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 389d668
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java bd3c4e7
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 94bc3a3
> exec/java-exec/src/main/java/org/apache/drill/exec/proto/helper/QueryIdHelper.java 1aadaa2
> exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
> exec/java-exec/src/main/java/org/apache/drill/exec/record/FragmentWritableBatch.java 7d157fe
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataResponseHandlerImpl.java 33e0665
> exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java c15bb7c
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/Comparators.java e9024ff
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java 3a66327
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileWrapper.java 80016aa
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/TableBuilder.java 72f4436
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java 3a7123d
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/UnlimitedRawBatchBuffer.java 2430e64
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java f824b53
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 31b1f2b
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/AbstractStatusReporter.java 4ff28f3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a4a97c9
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 7a819c4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java 41e87cd
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 84071c3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StateTransitionException.java 7155d43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StatusReporter.java 2bba345
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/SimpleRootExec.java 933417e
> protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java d9dba6e
> protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java acd8624
> protocol/src/main/java/org/apache/drill/exec/proto/beans/FragmentState.java ba536fc
> protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
> protocol/src/main/protobuf/UserBitShared.proto 7383bd2
>
> Diff: https://reviews.apache.org/r/33115/diff/
>
>
> Testing
> -------
>
> Regression & Unit, more manual testing planned before final patch.
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/
-----------------------------------------------------------
(Updated April 17, 2015, 5:04 p.m.)
Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
Changes
-------
small state change management improvements.
Repository: drill-git
Description
-------
DRILL-2762: Update Fragment state reporting and error collection
DeferredException
- Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
- Add new getAndClear operation
BufferManager
- Ensure close() can be called multiple times by clearing managed buffer list on close().
FragmentContext/FragmentExecutor
- Update FragmentContext to have a preClose so that we can check closure state before doing final close.
- Update so that there is only a single state maintained between FragmentContext and FragmentExecutor
- Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
- Add new CANCELLATION_REQUESTED state for FragmentState.
- Move all users of isCancelled or isFailed in main code to use shouldContinue()
- Update receivingFragmentFinished message to not cancel fragment (only inform root operator of cancellation)
WorkManager Updates
- Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
Foreman/QueryManager
- Extract listenable interfaces into anonymous inner classes from body of Foreman
QueryManager
- Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
- Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
FragmentData/MinorFragmentProfile
- Add ability to track last status update as well as last time fragment made progress
AbstractRecordBatch
- Update awareness of current cancellation state to avoid cancellation delays
Misc. Other changes
- Move ByteCode optimization code to only record assembly and code as trace messages
- Update SimpleRootExec to create fake ExecutorState to make existing tests work.
- Update sort to exit prematurely in the case that the fragment was asked to cancel.
- Add finals to all edited files.
- Modify control handler and FragmentManager to directly support receivingFragmentFinished
Diffs (updated)
-----
common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
common/src/main/java/org/apache/drill/common/concurrent/ExtendedLatch.java PRE-CREATION
common/src/main/java/org/apache/drill/common/exceptions/UserException.java e995346
exec/java-exec/src/main/java/org/apache/drill/exec/compile/AsmUtil.java 81904df
exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java 6c0292e
exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java 6e981bc
exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 44ca78a
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java 5b7ca66
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java e230fd2
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/producer/ProducerConsumerBatch.java c50cb8a
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 4b317e0
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 389d668
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java bd3c4e7
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 94bc3a3
exec/java-exec/src/main/java/org/apache/drill/exec/proto/helper/QueryIdHelper.java 1aadaa2
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
exec/java-exec/src/main/java/org/apache/drill/exec/record/FragmentWritableBatch.java 7d157fe
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataResponseHandlerImpl.java 33e0665
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java c15bb7c
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/Comparators.java e9024ff
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java 3a66327
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileWrapper.java 80016aa
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/TableBuilder.java 72f4436
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java 3a7123d
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/UnlimitedRawBatchBuffer.java 2430e64
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java f824b53
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 31b1f2b
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/AbstractStatusReporter.java 4ff28f3
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a4a97c9
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 7a819c4
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java 41e87cd
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 84071c3
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StateTransitionException.java 7155d43
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StatusReporter.java 2bba345
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/SimpleRootExec.java 933417e
protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java d9dba6e
protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java acd8624
protocol/src/main/java/org/apache/drill/exec/proto/beans/FragmentState.java ba536fc
protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
protocol/src/main/protobuf/UserBitShared.proto 7383bd2
Diff: https://reviews.apache.org/r/33115/diff/
Testing
-------
Regression & Unit, more manual testing planned before final patch.
Thanks,
Jacques Nadeau
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/
-----------------------------------------------------------
(Updated April 15, 2015, 7 p.m.)
Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
Changes
-------
Actually update patch
Repository: drill-git
Description
-------
DRILL-2762: Update Fragment state reporting and error collection
DeferredException
- Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
- Add new getAndClear operation
BufferManager
- Ensure close() can be called multiple times by clearing managed buffer list on close().
FragmentContext/FragmentExecutor
- Update FragmentContext to have a preClose so that we can check closure state before doing final close.
- Update so that there is only a single state maintained between FragmentContext and FragmentExecutor
- Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
- Add new CANCELLATION_REQUESTED state for FragmentState.
- Move all users of isCancelled or isFailed in main code to use shouldContinue()
- Update receivingFragmentFinished message to not cancel fragment (only inform root operator of cancellation)
WorkManager Updates
- Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
Foreman/QueryManager
- Extract listenable interfaces into anonymous inner classes from body of Foreman
QueryManager
- Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
- Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
FragmentData/MinorFragmentProfile
- Add ability to track last status update as well as last time fragment made progress
AbstractRecordBatch
- Update awareness of current cancellation state to avoid cancellation delays
Misc. Other changes
- Move ByteCode optimization code to only record assembly and code as trace messages
- Update SimpleRootExec to create fake ExecutorState to make existing tests work.
- Update sort to exit prematurely in the case that the fragment was asked to cancel.
- Add finals to all edited files.
- Modify control handler and FragmentManager to directly support receivingFragmentFinished
Diffs (updated)
-----
common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
exec/java-exec/src/main/java/org/apache/drill/exec/compile/AsmUtil.java 81904df
exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java 6c0292e
exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java 6e981bc
exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java 5b7ca66
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java e230fd2
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/producer/ProducerConsumerBatch.java c50cb8a
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 4b317e0
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 389d668
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java a23780e
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 94bc3a3
exec/java-exec/src/main/java/org/apache/drill/exec/proto/helper/QueryIdHelper.java 1aadaa2
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java c15bb7c
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java 3a7123d
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/UnlimitedRawBatchBuffer.java 2430e64
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/AbstractStatusReporter.java 1b0885d
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 7a819c4
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java 41e87cd
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 84071c3
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StateTransitionException.java 7155d43
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StatusReporter.java 26b5d68
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/SimpleRootExec.java 933417e
protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 96a921b
protocol/src/main/java/org/apache/drill/exec/proto/beans/FragmentState.java ba536fc
protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
protocol/src/main/protobuf/UserBitShared.proto 2938114
Diff: https://reviews.apache.org/r/33115/diff/
Testing
-------
Regression & Unit, more manual testing planned before final patch.
Thanks,
Jacques Nadeau
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/
-----------------------------------------------------------
(Updated April 15, 2015, 5:22 a.m.)
Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
Changes
-------
Updates to clarify FragmentExecutor and FragmentContext state. See updated description for explanation of changeset (as some things were removed from previous patch).
Repository: drill-git
Description (updated)
-------
DRILL-2762: Update Fragment state reporting and error collection
DeferredException
- Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
- Add new getAndClear operation
BufferManager
- Ensure close() can be called multiple times by clearing managed buffer list on close().
FragmentContext/FragmentExecutor
- Update FragmentContext to have a preClose so that we can check closure state before doing final close.
- Update so that there is only a single state maintained between FragmentContext and FragmentExecutor
- Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
- Add new CANCELLATION_REQUESTED state for FragmentState.
- Move all users of isCancelled or isFailed in main code to use shouldContinue()
- Update receivingFragmentFinished message to not cancel fragment (only inform root operator of cancellation)
WorkManager Updates
- Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
Foreman/QueryManager
- Extract listenable interfaces into anonymous inner classes from body of Foreman
QueryManager
- Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
- Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
FragmentData/MinorFragmentProfile
- Add ability to track last status update as well as last time fragment made progress
AbstractRecordBatch
- Update awareness of current cancellation state to avoid cancellation delays
Misc. Other changes
- Move ByteCode optimization code to only record assembly and code as trace messages
- Update SimpleRootExec to create fake ExecutorState to make existing tests work.
- Update sort to exit prematurely in the case that the fragment was asked to cancel.
- Add finals to all edited files.
- Modify control handler and FragmentManager to directly support receivingFragmentFinished
Diffs (updated)
-----
common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java ac1bcbb
protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
protocol/src/main/protobuf/UserBitShared.proto 2938114
Diff: https://reviews.apache.org/r/33115/diff/
Testing
-------
Regression & Unit, more manual testing planned before final patch.
Thanks,
Jacques Nadeau
Re: Review Request 33115: DRILL-2762: Update Fragment state reporting
and error collection
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33115/#review79830
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/33115/#comment129421>
Need to complete javadoc
- Jacques Nadeau
On April 13, 2015, 12:07 a.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33115/
> -----------------------------------------------------------
>
> (Updated April 13, 2015, 12:07 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2762: Update Fragment state reporting and error collection
>
> DeferredException
> - Add new throwAndClear operation on to allow checking for exceptions preClose in FragmentContext
>
> BufferManager
> - Ensure close() can be called multiple times by clearing managed buffer list on close().
>
> FragmentContext/FragmentExecutor
> - Update FragmentContext to have a preClose so that we can check closure state before doing final close.
> - Clean up FragmentExecutor run() method to better manage error states and have only single terminal point (avoiding multiple messages to Foreman).
>
> WorkManager Updates
> - Add new afterExecute command to the WorkManager ExecutorService so that we get log entries if a thread leaks an exception. (Otherwise logs don't show these exceptions and they only go to standard out.)
> - Change status thread so it only reports non-terminal status to the Foreman to avoid race conditions and confusion when receiving multiple possibly incomplete terminal messages.
>
> Foreman/QueryManager
> - Extract listenable interfaces into anonymous inner classes from body of Foreman
>
> QueryManager
> - Update QueryManager to track completed nodes rather than completed fragments using NodeTracker
> - Update DrillbitStatusListener to decrement expected completion messages on Nodes that have died to avoid query hang when a node dies
>
> FragmentData/MinorFragmentProfile
> - Add ability to track last status update as well as last time fragment made progress
>
> AbstractRecordBatch
> - Update awareness of current cancellation state to avoid cancellation delays
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/DeferredException.java 99f18f1
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java 2d22d84
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c
> exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java 2bb29e5
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java e2bcec3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java 433ab26
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a7e6c46
> protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1
> protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java ac1bcbb
> protocol/src/main/java/org/apache/drill/exec/proto/beans/MinorFragmentProfile.java 5cd71f9
> protocol/src/main/protobuf/UserBitShared.proto 2938114
>
> Diff: https://reviews.apache.org/r/33115/diff/
>
>
> Testing
> -------
>
> Regression & Unit, more manual testing planned before final patch.
>
>
> Thanks,
>
> Jacques Nadeau
>
>