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