----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18009/#review34363 -----------------------------------------------------------
Great job! Especially, I love the design of RpcChannelFactory and the change of Fetcher and RpcConnectionPool. It would manages efficiently the number of running thread. This patch also contains other refactorings. I usually leaved comments on them. tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.java <https://reviews.apache.org/r/18009/#comment64439> You may intend to use hadoop.io.IOUtils. tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/TajoAsyncDispatcher.java <https://reviews.apache.org/r/18009/#comment64441> If you give some explain about the reason to change the blocking queue to concurrent linked queue, it would be helpful for reviewing this patch. tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/TajoAsyncDispatcher.java <https://reviews.apache.org/r/18009/#comment64440> Since the queue is changed to ConcurrentLinkedQueue, the eventQueue.size() will be expensive. Due to its nature, ConcurrentLinkedQueue::size() should traverse items in the queue in order to get size(). Especially, the size of eventQueue can be tens of thousands during task scheduling. So, it may cause some overheads. tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/worker/Fetcher.java <https://reviews.apache.org/r/18009/#comment64442> it looks nice work. tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcServer.java <https://reviews.apache.org/r/18009/#comment64449> In other classes, you use workerNum instead of ioThreadNum. If you rename ioThreadNum to workerNum, it would be more consistent throughout the code. tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcServer.java <https://reviews.apache.org/r/18009/#comment64452> This is also the rename issue aforementioned. tajo-rpc/src/main/java/org/apache/tajo/rpc/BlockingRpcServer.java <https://reviews.apache.org/r/18009/#comment64453> This is also the rename issue aforementioned. tajo-rpc/src/main/java/org/apache/tajo/rpc/BlockingRpcServer.java <https://reviews.apache.org/r/18009/#comment64454> This is also the rename issue aforementioned. tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcChannelFactory.java <https://reviews.apache.org/r/18009/#comment64455> This is also the rename issue aforementioned. - Hyunsik Choi On Feb. 13, 2014, 12:07 a.m., Jinho Kim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18009/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2014, 12:07 a.m.) > > > Review request for Tajo. > > > Bugs: TAJO-598 > https://issues.apache.org/jira/browse/TAJO-598 > > > Repository: tajo > > > Description > ------- > > In the current implementation, all client rpc use same channel pool. > It can cause a channel closed exception. we need both shared pool and new > pool. > In details, > * Fix the TajoAsyncDispatcher hang > * Fix the Fetcher timeout > * Fix the TaskRunner thread leak > * Fix the client rpc reconnecting > * Fix the unittest failure(No available resources) > * Improve RPC thread sharing > > > Diffs > ----- > > > tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/CatalogServer.java > 62d6e27355ed0b542850af7b9240f5f504c1631b > tajo-client/src/main/java/org/apache/tajo/cli/TajoCli.java > be4be3f3c727ccbcf8f939fdd26eff0ea3e46def > tajo-client/src/main/java/org/apache/tajo/client/TajoClient.java > 2846398861c226c725a3f311247c9fd0c50bb114 > tajo-common/src/main/java/org/apache/tajo/conf/TajoConf.java > 7c82d72d5dfb8ea4c99367ec86e966b1505887cd > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/benchmark/SimpleQuery.java > fb8fe5f129ad438b5f750b785d9a45cb8126f2da > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/benchmark/TPCH.java > d11941d93f06b38021a3e20afbb9e64b44243bf2 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/AggregationExec.java > eb697030b1e6dc51c1e845c4261ec33011fdcbd4 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/BNLJoinExec.java > b39a9f1ee15fb88e13d98259c6b1759927073923 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/BSTIndexScanExec.java > 2ff6fc965e9db66ba37a57ecadd7a23053086f2e > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/BinaryPhysicalExec.java > f1e3a004dcbcdcc87215ba90ad172024d3e93daa > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/ExternalSortExec.java > f0ac2906dc487daf26281dad23e5ce7ac22ccd50 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/HashAggregateExec.java > a4153dc8cd4e0191cbbf071e1a650e031b958263 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/HashFullOuterJoinExec.java > 848e36252137bdef2fef7023daf629e674e1e63d > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/HashJoinExec.java > 08b70351d3d1dc18dc4f3e277132458b51ddd9d2 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/HashLeftOuterJoinExec.java > 314b3d975b3399ccfd887dc763ed1c7f023316f4 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/HashShuffleFileWriteExec.java > c09ec19edd20b0f495749f61e846edffb9c9b142 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/MemSortExec.java > 46d061ab515d56bb7595e9380d9f50dcde699bed > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/MergeFullOuterJoinExec.java > 1d6da3fb2c62b252d27e332d09f109a3859b7bb5 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/MergeJoinExec.java > e1d377e57d7a2bc487267950f8fe8be641316f4f > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/ProjectionExec.java > ecc6dd0bc7543bc1bf5eee22fbe28547cac4461b > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/RangeShuffleFileWriteExec.java > 13573ebc33ee31ff9086833c620864dc0096ee3a > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/RightOuterMergeJoinExec.java > 365fabaac66190fd2cd43e14a1cba02dbb0199cd > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.java > 31a944c2906c183de69b7ef08ec99b4c5282aeff > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/StoreTableExec.java > f097d0c393384aadfdd3188ff2a58a6161d0b624 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/UnaryPhysicalExec.java > f31455f456395bfdae0675eb953970bab337a70b > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/DefaultTaskScheduler.java > b6bde943ff9b76f5e410aed3273f41634981284a > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/LazyTaskScheduler.java > 08d080d8299d575caa9a93eceba855fcf666e086 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/TajoAsyncDispatcher.java > 751b21bd23e63ea54f818d03b4d4c493e67a03b4 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/TajoContainerProxy.java > 39a73ba82700bce96cc084c2cfb73038f3e41ab7 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/TajoMaster.java > 2c194fdc873567d45f34622e2a369c6e8e9b6275 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/TajoMasterClientService.java > 6e2ee8b171f9250b76e50383d032237b609244d1 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/TajoMasterService.java > 7853f6376f7a53d2fd378c13390e064099a7c159 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/YarnContainerProxy.java > dbec65276b0822ab5166dbe6f139e54a25d26249 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/YarnTaskRunnerLauncherImpl.java > 7c38e7af901d090dabeaa2906021bf80626cbf7e > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/querymaster/QueryInProgress.java > 2ab6c6a22412848053b0a3834959c3623648dbba > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/querymaster/QueryMaster.java > ecf6cb2175b7d52235ba64db80b3b7c11fe67917 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/querymaster/QueryMasterManagerService.java > 3618d3b6b0cd13bd28cc47262d718824d2cffa39 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/querymaster/QueryMasterTask.java > e193509192732194cd09db938a2e0a2660270100 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/rm/TajoWorkerResourceManager.java > b39901e284c24d717a69a79f2bb97a398ecc0871 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/rm/YarnTajoResourceManager.java > 72ada9b10c5b7826e4b079291f7212f924f3545f > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/util/metrics/reporter/MetricsFileScheduledReporter.java > 35dd6f186fbfaa73c7ab2c7f9b78c6ae5b458905 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/util/metrics/reporter/TajoMetricsScheduledReporter.java > c9d25c5d2b0e258fa910529ac739afd8c14daf98 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/worker/Fetcher.java > 4f9f3fa1581ab6e4ea13923d5f235fc025d79315 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/worker/TajoResourceAllocator.java > 6d01f552fa4c59fe717f4d77c7befb4b791f8430 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/worker/TajoWorker.java > 456bd95ab877d2b110de7e6fc1fd0ca2cf0f1c81 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/worker/TajoWorkerClientService.java > f6e7742054e7be35ddf5edaed7c033d33d8283cd > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/worker/TajoWorkerManagerService.java > 60b09036895b22739d607f947c09942d1a408c28 > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/worker/Task.java > 8a0b63a061f43baf742713e5bbcc8a181c2af002 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/worker/TaskRunner.java > dab18b586fd5d06c2cb257a3ca654ff24d5efad1 > > tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/eval/TestEvalTree.java > 0489e379cf15d5769dfd9cf797ad30e28f78c738 > > tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/eval/TestEvalTreeUtil.java > c25afc87fd2bfc1b626e41f3bf285da35f80ef34 > > tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/master/TestGlobalPlanner.java > 3cf69a72f4bce0dbf3d73c09ffa289d95299a48b > > tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/worker/TestFetcher.java > 0181889d3a2f4fd2ffd98eba8ed61a8f21e66bdd > > tajo-core/tajo-core-pullserver/src/main/java/org/apache/tajo/pullserver/TajoPullServerService.java > da34ac74d1eafc63b38b6fa5d0fec0420039a5c4 > tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcClient.java > 01e1110e6f4e9405761d35bae265d2c8fe553456 > tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcServer.java > 4dc1d052b33911ee69450061e048df5df0e1884e > tajo-rpc/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java > a0963a76fc566c35fc4fd734d6f21d0cd81877ab > tajo-rpc/src/main/java/org/apache/tajo/rpc/BlockingRpcServer.java > 3649c5e18fa9465b807c6fc3638de0a8b04bf7f2 > tajo-rpc/src/main/java/org/apache/tajo/rpc/NettyClientBase.java > 79c26747d7a366dc665f36f85a2abca55db67958 > tajo-rpc/src/main/java/org/apache/tajo/rpc/NettyServerBase.java > 154fb9b3e96bfa4e6a62bd595cff1eae544c0972 > tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcChannelFactory.java > PRE-CREATION > tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcConnectionPool.java > dee42d1d3634356e64b6a1254194684431a79642 > tajo-rpc/src/test/java/org/apache/tajo/rpc/TestAsyncRpc.java > 83914e5fc2164d5a1b2548cb8e513860a29a3c84 > tajo-rpc/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java > 6cd5f251782b230c66cf36590bebfcd32009a048 > > Diff: https://reviews.apache.org/r/18009/diff/ > > > Testing > ------- > > > Thanks, > > Jinho Kim > >
