----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31435/#review75796 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java <https://reviews.apache.org/r/31435/#comment123086> I'm speaking about the Web UI rather than the client. If you cancel a query, then it should show up as cancelled in the WebUI. Currently, it does. With this change, it wouldn't. exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java <https://reviews.apache.org/r/31435/#comment123089> We can enter this code in multiple scenarios. One is if the query succeeded, another when the query failed. If the body of the query succeeded and then we failed to clean up, we should propogate that back to the end user. However, if the query already failed and then we fail to cleanup resources, the primary thing we should propogate to the user is that query failed, not the cleanup problem. I propose that to make this cleaner, we update the failure cause and FragmentContext to be able to add multiple exceptions via suppressed exceptions. That way we can follow the approach of first exception is root cause, subsequent exceptions are added as suppressed exceptions. We should do that across the code over time. - Jacques Nadeau On March 4, 2015, 12:21 a.m., Chris Westin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31435/ > ----------------------------------------------------------- > > (Updated March 4, 2015, 12:21 a.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 > >
