> On May 29, 2014, 6:40 p.m., Pavan Kumar Athivarapu wrote: > > findbugs-exclude.xml, line 46 > > <https://reviews.apache.org/r/21987/diff/1/?file=597788#file597788line46> > > > > why are u removing these stuff, > > wouldn't it be fine to just add GraphTaskManager to allow exit
I just removed unused code from GiraphYarnTask and hence it doesn't have System.exit() anymore and that made this check obsolete. > On May 29, 2014, 6:40 p.m., Pavan Kumar Athivarapu wrote: > > giraph-core/src/main/java/org/apache/giraph/comm/netty/handler/ExceptionHandler.java, > > line 29 > > <https://reviews.apache.org/r/21987/diff/1/?file=597792#file597792line29> > > > > this catches exceptions only for inbound conenctions, do u want to do > > this for outbound as well. > > > > in that case would it not be better to just piggy back on the counters > > & override exceptioncaught in those methods? > > > > I am fine with creating new classes or just overriding the method in > > counters > > Pavan Kumar Athivarapu wrote: > one more thing (for exception handling) - inbound handler should be at > the end and outbound handler should be at the beginning of the pipeline > so u catch exceptions at the end of processing input - 1 > u catch exceptions at the end of sending output - 2 I felt like it's nice to have a separate class for exception handling as it outlines it's intention and makes it clear how and when to use it. - Sergey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21987/#review44270 ----------------------------------------------------------- On June 5, 2014, 10:26 p.m., Sergey Edunov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21987/ > ----------------------------------------------------------- > > (Updated June 5, 2014, 10:26 p.m.) > > > Review request for giraph. > > > Repository: giraph-git > > > Description > ------- > > When some of the request processing threads fails, the worker gets stuck but > the job doesn't fail and it has to be killed manually. We should detect netty > thread crashes and fail the job automatically. > > > Diffs > ----- > > findbugs-exclude.xml e0466f7 > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyClient.java > ae40c3b > > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyMasterClient.java > c982209 > > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyMasterServer.java > cb36c3e > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyServer.java > 14d4ea8 > > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClient.java > 7541418 > > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerServer.java > adb96cb > > giraph-core/src/main/java/org/apache/giraph/comm/netty/handler/ExceptionHandler.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/comm/netty/handler/RequestServerHandler.java > 601cd2f > giraph-core/src/main/java/org/apache/giraph/graph/GraphMapper.java c86a024 > giraph-core/src/main/java/org/apache/giraph/graph/GraphTaskManager.java > ad5fc91 > giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java > 90dc9f3 > giraph-core/src/main/java/org/apache/giraph/worker/BspServiceWorker.java > aff7084 > giraph-core/src/main/java/org/apache/giraph/yarn/GiraphYarnTask.java > f4719cc > giraph-core/src/test/java/org/apache/giraph/comm/ConnectionTest.java > e771e36 > giraph-core/src/test/java/org/apache/giraph/comm/MockExceptionHandler.java > PRE-CREATION > giraph-core/src/test/java/org/apache/giraph/comm/RequestFailureTest.java > 236bc88 > giraph-core/src/test/java/org/apache/giraph/comm/RequestTest.java fcdfa5c > giraph-core/src/test/java/org/apache/giraph/comm/SaslConnectionTest.java > c026cf8 > > Diff: https://reviews.apache.org/r/21987/diff/ > > > Testing > ------- > > Run some production jobs with this change. > Also introduced random bugs in deserialization logic and confirmed that job > fails. > > > Thanks, > > Sergey Edunov > >
