Return-Path: X-Original-To: apmail-giraph-dev-archive@www.apache.org Delivered-To: apmail-giraph-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 0D73FDDFF for ; Tue, 21 Aug 2012 22:03:11 +0000 (UTC) Received: (qmail 22867 invoked by uid 500); 21 Aug 2012 22:03:10 -0000 Delivered-To: apmail-giraph-dev-archive@giraph.apache.org Received: (qmail 22825 invoked by uid 500); 21 Aug 2012 22:03:10 -0000 Mailing-List: contact dev-help@giraph.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@giraph.apache.org Delivered-To: mailing list dev@giraph.apache.org Delivered-To: moderator for dev@giraph.apache.org Received: (qmail 19531 invoked by uid 99); 21 Aug 2012 22:01:07 -0000 Content-Type: multipart/alternative; boundary="===============2420671436112023357==" MIME-Version: 1.0 Subject: Re: Review Request: GIRAPH-306: Netty requests should be reliable and implement exactly once semantics From: "Alessandro Presta" To: "Avery Ching" , "giraph" , "Alessandro Presta" Date: Tue, 21 Aug 2012 22:01:05 -0000 Message-ID: <20120821220105.15256.40706@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Alessandro Presta" X-ReviewGroup: giraph X-ReviewRequest-URL: https://reviews.apache.org/r/6687/ X-Sender: "Alessandro Presta" References: <20120821165301.15255.90650@reviews.apache.org> In-Reply-To: <20120821165301.15255.90650@reviews.apache.org> Reply-To: "Alessandro Presta" --===============2420671436112023357== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On Aug. 21, 2012, 4:53 p.m., Alessandro Presta wrote: > > Really cool! It looks like some network failures are unavoidable scalin= g up, so resending requests on one end and ensuring unicity on the other se= ems a solid approach. > > From your numbers, it's a pretty dramatic step up and I can't wait to t= ry it. > > = > > Is there any significant impact on jobs that wouldn't require the addit= ional precautions? In other words, if you take a job that succeeds with cur= rent trunk, do the communication-heavy phases take considerably longer? > > = > > Other than that, only some comments about maintainability. > > = > > Thanks! > = > Avery Ching wrote: > The additional overhead is an additional integer that is send/receive= d with every request (small price to pay for reliability and also small whe= n we are bulking requests). Running tests now, but don't expect any notice= able differences. I was referring more to run time, wondering if the additional logic (e.g. i= terating over all requests to check the connection in waitSomeRequests) pen= alizes us significantly. The one integer per request seems unimportant to m= e (as you say, we batch up messages/vertices in requests). The benchmarks you posted on the JIRA have indeed high variance, so we can'= t say much except that the difference is not dramatic. I also think reliabi= lity matters more at this stage, especially since we have consistent failur= es over a certain number of workers. Ship it! > On Aug. 21, 2012, 4:53 p.m., Alessandro Presta wrote: > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/WorkerIdRequestId.java, line 24 > > > > > > Just a matter of taste, but I think we could use a more meaningful = name here. > > Some ideas: GlobalRequestId (as this is a global, unique identifier= ), WorkerRequestIdPair (super-explicit). > > Also I would specify in the comments that we're referring to the de= stination worker. > = > Avery Ching wrote: > I agree. I changed the same to ClientRequestId since it is not globa= lly unique, but rather unique to a client. Oh right. Sounds good. > On Aug. 21, 2012, 4:53 p.m., Alessandro Presta wrote: > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/NettyClient.java, line 211 > > > > > > I would turn this 13 into a named constant and briefly explain how = you get to this number, otherwise this is difficult to maintain. > = > Avery Ching wrote: > Fixed. This is a good idea. Actually my bad here, I didn't realize it's simply 4 (int) + 8 (long) + 1 (= byte). So I guess it wasn't a big deal, sorry. > On Aug. 21, 2012, 4:53 p.m., Alessandro Presta wrote: > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/RequestServerHandler.java, line 91 > > > > > > I normally don't like having test code paths in non-test classes. I= s there absolutely no way around this? > = > Avery Ching wrote: > I can understand the apprehension, but testing failure scenarios is a= bit tricky without in-code instrumentation. The alternative would be to r= efactor the logic and try to mock it in, but this is a bit yucky as well, s= ince we would only be factoring for testing the failure, not for any other = purpose. Yeah, I see what you're saying. It's alright then. - Alessandro ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6687/#review10573 ----------------------------------------------------------- On Aug. 21, 2012, 7:47 p.m., Avery Ching wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6687/ > ----------------------------------------------------------- > = > (Updated Aug. 21, 2012, 7:47 p.m.) > = > = > Review request for giraph. > = > = > Description > ------- > = > One of the biggest scalability challenges is getting Giraph to run reliab= ly on a large number of tasks (i.e. > 200). Several problems exist: > = > 1) If the connection fails after the initial connection was made, the job= will die. > 2) Requests must be completed exactly once. This is difficult to implemen= t, but required since we cannot have multiple retried requests succeed (i.e= . a vertex gets more messages than expected). > 3) Sometimes there are unresolved addresses, causing failure. > = > This patch addresses these issues by re-establishing failed connections a= nd keep tracking of every request sent to every worker. If the request fail= s or passes a timeout, it will be resent. The server will keep track of req= uests that succeeded to insure that the same request won't be processed mor= e than once. The structure for keeping track of the succeeded requests on t= he server is efficient for handling increasing request ids (IncreasingBitSe= t). For handling unresolved addresses, I added retry logic to keep trying t= o resolve the problem. > = > This patch also adds several unit tests that use fault injection to simul= ate a lost response or a closed channel exception on the server. It also ha= s unittests for IncreasingBitSet to insure it is working correctly and effi= ciently. > = > This passes all unittests (including the new ones). Additionally, I have = some experience results as well. > = > Previously, I was unable to run reliably with more than 200 workers. With= this change I can reliably run 500+ workers. I also ran with 600 workers s= uccessfully. This is a really big reliability win for us. > = > I can see the code working to do reconnections and re-issue requests when= necessary. It's very cool. > = > I.e. > = > 2012-08-18 00:16:52,109 INFO org.apache.giraph.comm.NettyClient: checkAnd= FixChannel: Fixing disconnected channel to xxx.xxx.xxx.xxx/xx.xx.xx.xx:3045= 5, open =3D false, bound =3D false > 2012-08-18 00:16:52,111 INFO org.apache.giraph.comm.NettyClient: checkAnd= FixChannel: Connected to xxx.xxx.xxx.xxx/xxx.xxx.xxx.xxx:30455! > 2012-08-18 00:16:52,123 INFO org.apache.giraph.comm.NettyClient: checkAnd= FixChannel: Fixing disconnected channel to xxx.xxx.xxx.xxx/xxx.xxx.xxx.xxx,= open =3D false, bound =3D false > 2012-08-18 00:16:52,124 INFO org.apache.giraph.comm.NettyClient: checkAnd= FixChannel: Connected to xxx.xxx.xxx.xxx/xxx.xxx.xxx.xxx:30117! > = > = > This addresses bug GIRAPH-306. > https://issues.apache.org/jira/browse/GIRAPH-306 > = > = > Diffs > ----- > = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/AddressRequestIdGenerator.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/ChannelRotater.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/ClientRequestId.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/IncreasingBitSet.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/NettyClient.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/NettyServer.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/NettyWorkerClient.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/RequestDecoder.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/RequestInfo.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/RequestServerHandler.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/ResponseClientHandler.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/WorkerRequestReservedMap.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/comm/WritableRequest.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/BspServiceMaster.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/BspServiceWorker.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/GiraphJob.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/WorkerInfo.java 1375393 = > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/g= iraph/comm/IncreasingBitSetTest.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/g= iraph/comm/RequestFailureTest.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/g= iraph/comm/RequestTest.java 1375393 = > = > Diff: https://reviews.apache.org/r/6687/diff/ > = > = > Testing > ------- > = > mvn clean verify > Lots of large test 500-600 workers with PageRankBenchmark > = > = > Thanks, > = > Avery Ching > = > --===============2420671436112023357==--