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