drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Westin" <chriswesti...@gmail.com>
Subject Re: Review Request 33115: DRILL-2762: Update Fragment state reporting and error collection
Date Fri, 17 Apr 2015 18:48:51 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message