drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jacques Nadeau" <jacques.dr...@gmail.com>
Subject Re: Review Request 31435: DRILL-2245-core: core changes for improved drillbit stability
Date Fri, 27 Feb 2015 05:56:09 GMT

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


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