----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31435/#review74282 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java <https://reviews.apache.org/r/31435/#comment120827> Did you reorder for a sepcific reason? Ideally we should unregister from the coordination service before shutting things down. In fact, we should probably also pause before shutting things down after unregistering exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java <https://reviews.apache.org/r/31435/#comment120830> Do you make sure that Foremans no longer leak exceptions? This was here to ensure that we get an error rather than the thread terminating and the error showing up in standard err. exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java <https://reviews.apache.org/r/31435/#comment120838> is this guaranteed to be called or can something above prematurely end this method via exception? exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java <https://reviews.apache.org/r/31435/#comment120893> can we just use if this logger.isInfoEnabled()? exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java <https://reviews.apache.org/r/31435/#comment120896> Canceled should be a terminal state from the user's perspective. Not sure that happens here or if you manage this some other way. exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java <https://reviews.apache.org/r/31435/#comment120897> Shouldn't this be protected? exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java <https://reviews.apache.org/r/31435/#comment120898> Can we make this be the first exception with rest of the exceptions tacked on as suppressed exceptions? exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java <https://reviews.apache.org/r/31435/#comment121013> can you pick message for the assertion so if we hit it, we get something other than "null" exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java <https://reviews.apache.org/r/31435/#comment121014> No, but that is on purpose. We keep them forever for log analysis exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java <https://reviews.apache.org/r/31435/#comment121023> We need to make sure we have everything closed before returning success. You've removed that functionality by eating (only logging) the exceptions. If a query leaks anything, I want the query to fail so it shows up in all the tests. I'm okay disabling this functionality in production mode but definitely want it to be the case in normal development and testing. If you disagree, we should discuss further because I see this a show stopper for this patch. exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java <https://reviews.apache.org/r/31435/#comment121026> This is a stylistic change. I think you need to propose this on the list and we come to a group consensus. As it is, this is inconsistent with the whole rest of the code base. Please discuss on the dev list before changing this everywhere. exec/java-exec/src/test/java/org/apache/drill/exec/TestWithZookeeper.java <https://reviews.apache.org/r/31435/#comment121032> why commented? I see no downside to having an not-yet used logger in a class. - Jacques Nadeau On Feb. 25, 2015, 9:42 p.m., Chris Westin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31435/ > ----------------------------------------------------------- > > (Updated Feb. 25, 2015, 9:42 p.m.) > > > Review request for drill and Jacques Nadeau. > > > Bugs: DRILL-2245 > https://issues.apache.org/jira/browse/DRILL-2245 > > > Repository: drill-git > > > Description > ------- > > DRILL-2245-core: Clean up query setup and execution kickoff in > Foreman/WorkManager in order to ensure consistent handling, and avoid hangs > and races, with the goal of improving Drillbit robustness. > > I did my best to keep these clean when I split them up, but this core > commit > may depend on some minor changes in the hygiene commit that is also > associated with this bug, so either both should be applied, or neither. > The core commit should be applied first. > > AutoCloseables > - created org.apache.drill.common.AutoCloseables to handle closing these > quietly > > BaseTestQuery, and derivatives > - factored out pieces into QueryTestUtil so they can be reused > > Drillbit > - uses AutoCloseables for the WorkManager and for the storeProvider > - allow start() to take a RemoteServiceSet > - private, final, formatting > > Foreman > - does not need to implement Comparable > - does not need to implement Closeable > - thread blocking fixes > - add resultSent flag > - add code to log plan fragments with endpoint assignments > - added finals, cleaned up formatting > - do queue management in acquireQuerySemaphore; local tests pass > - rename getContext() to getQueryContext() > - retain DrillbitContext > - a couple of exception injections for testing > - minor formatting > - TODOs > > FragmentExecutor > - eliminated CancelableQuery > - common subexpression elimination > - cleaned up > > QueryContext > - removed unnecessary functions (with some outside classes tweaked for > this) > - finals, formatting > > QueryManager > - merge in QueryStatus > - affects Foreman, ../batch/ControlHandlerImpl, > and ../../server/rest/ProfileResources > - made some methods private > - removed unused imports > - add finals and formatting > - variable renaming to improve readability > - formatting > - comments > - TODOs > > QueryStatus > - getAsInfo() private > - member renaming > - member access changes > - formatting > - TODOs > > QueryTestUtil, BaseTestQuery, TestDrillbitResilience > - make maxWidth a parameter to server startup > > SelfCleaningRunnable > - created org.apache.drill.common.SelfCleaningRunnable > > SingleRowListener > - created org.apache.drill.SingleRowListener results listener > - use in TestDrillbitResilience > > TestDrillbitResilience > - created org.apache.drill.exec.server.TestDrillbitResilience to test > drillb > resilience in the face of exceptions and failures during queries > > TestWithZookeeper > - factor out work into ZookeeperHelper so that it can be reused by > TestDrillbitResilience > > UserBitShared > - get rid of unused UNKNOWN_QUERY > > WorkEventBus > - rename methods, affects Foreman and ControlHandlerImpl > - remove unused WorkerBee reference > - most members final > - formatting > > WorkManager > - Closeable to AutoCloseable > - removed unused incomingFragments Set > - eliminated unnecessary eventThread and pendingTasks by posting Runnables > directly to executor > - use SelfCleaningRunnable for Foreman management > - FragmentExecutor management uses SelfCleaningRunnable > - runningFragments to be a ConcurrentHashMap; TestTpchDistributed passes > - other improvements due to bee no longer needed in various places > - most members final > - minor formatting > - comments > - TODOs > > (*) Created exception injection classes to simulate exceptions for testing > - ExceptionInjection > - ExceptionInjector > - ExceptionInjectionUtil > - TestExceptionInjection > > > Diffs > ----- > > common/src/main/java/org/apache/drill/common/AutoCloseables.java > PRE-CREATION > common/src/main/java/org/apache/drill/common/SelfCleaningRunnable.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java > 5efcce8 > exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java > 04b955b > > exec/java-exec/src/main/java/org/apache/drill/exec/coord/local/LocalClusterCoordinator.java > 035c1aa > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/WorkEventBus.java > d6b8637 > exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java > 67342c4 > > exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java > 83a89df > > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java > aa0a5ad > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java > ae04bad > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExceptionInjection.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExceptionInjector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/InjectionSite.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/SimulatedExceptions.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/work/CancelableQuery.java > 5b11943 > exec/java-exec/src/main/java/org/apache/drill/exec/work/StatusProvider.java > 6086f74 > exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java > 99c6ab8 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java > 3228da9 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java > 378e81a > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java > 52fd0a9 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentStatusListener.java > 6a719d2 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java > 2de3592 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryStatus.java > 4e18da6 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java > 7ccb64e > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java > 3671804 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java > 54fc8c4 > exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java cf99577 > exec/java-exec/src/test/java/org/apache/drill/PlanTestBase.java c52545d > exec/java-exec/src/test/java/org/apache/drill/QueryTestUtil.java > PRE-CREATION > exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java > PRE-CREATION > exec/java-exec/src/test/java/org/apache/drill/TestBugFixes.java 51f3121 > exec/java-exec/src/test/java/org/apache/drill/TestBuilder.java 978e565 > exec/java-exec/src/test/java/org/apache/drill/exec/DrillSystemTestBase.java > 75ba3a9 > exec/java-exec/src/test/java/org/apache/drill/exec/TestWithZookeeper.java > bb69c9a > exec/java-exec/src/test/java/org/apache/drill/exec/ZookeeperHelper.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java > 4aaaa78 > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/ExceptionInjectionUtil.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java > PRE-CREATION > protocol/src/main/protobuf/UserBitShared.proto 8f05c45 > > Diff: https://reviews.apache.org/r/31435/diff/ > > > Testing > ------- > > mvn install > Functional - Passing - New > Advanced - TPCH SF100 - Parquet > > > Thanks, > > Chris Westin > >
