Return-Path: X-Original-To: apmail-trafficserver-dev-archive@www.apache.org Delivered-To: apmail-trafficserver-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 01F3C18F45 for ; Fri, 11 Dec 2015 18:27:38 +0000 (UTC) Received: (qmail 39550 invoked by uid 500); 11 Dec 2015 18:27:33 -0000 Delivered-To: apmail-trafficserver-dev-archive@trafficserver.apache.org Received: (qmail 39487 invoked by uid 500); 11 Dec 2015 18:27:33 -0000 Mailing-List: contact dev-help@trafficserver.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@trafficserver.apache.org Delivered-To: mailing list dev@trafficserver.apache.org Received: (qmail 39476 invoked by uid 99); 11 Dec 2015 18:27:32 -0000 Received: from Unknown (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 11 Dec 2015 18:27:32 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id 5800F180A9A for ; Fri, 11 Dec 2015 18:27:32 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 2.697 X-Spam-Level: ** X-Spam-Status: No, score=2.697 tagged_above=-999 required=6.31 tests=[HTML_MESSAGE=3, KAM_LOTSOFHASH=0.25, RP_MATCHES_RCVD=-0.554, URIBL_BLOCKED=0.001] autolearn=disabled Received: from mx1-us-west.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id BJ9d0HAH0kPS for ; Fri, 11 Dec 2015 18:27:18 +0000 (UTC) Received: from copdcmhout01.cable.comcast.com (copdcmhout01.cable.comcast.com [162.150.44.71]) by mx1-us-west.apache.org (ASF Mail Server at mx1-us-west.apache.org) with ESMTPS id 92ED9265F4 for ; Fri, 11 Dec 2015 18:27:18 +0000 (UTC) X-AuditID: a2962c47-f79c76d000001a8b-79-566b15854b2a Received: from COPDCEX12.cable.comcast.com (Unknown_Domain [96.114.156.147]) (using TLS with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by copdcmhout01.cable.comcast.com (SMTP Gateway) with SMTP id 4D.E0.06795.5851B665; Fri, 11 Dec 2015 11:27:17 -0700 (MST) Received: from VAADCEX40.cable.comcast.com (147.191.103.217) by COPDCEX12.cable.comcast.com (147.191.124.143) with Microsoft SMTP Server (TLS) id 15.0.1130.7; Fri, 11 Dec 2015 11:27:17 -0700 Received: from VAADCEX34.cable.comcast.com (147.191.103.211) by VAADCEX40.cable.comcast.com (147.191.103.217) with Microsoft SMTP Server (TLS) id 15.0.1130.7; Fri, 11 Dec 2015 13:27:15 -0500 Received: from VAADCEX34.cable.comcast.com ([fe80::3aea:a7ff:fe12:e198]) by VAADCEX34.cable.comcast.com ([fe80::3aea:a7ff:fe12:e198%19]) with mapi id 15.00.1130.005; Fri, 11 Dec 2015 13:27:15 -0500 From: "Rushford, John" To: James Peach CC: "dev@trafficserver.apache.org" Subject: Re: [trafficserver] TS-3418: Second hash ring for consistently hashed parent selection (#359) Thread-Topic: [trafficserver] TS-3418: Second hash ring for consistently hashed parent selection (#359) Thread-Index: AQHRMxAcOex/vhlSU0qvV+bwGFGtFp7F+3SA Date: Fri, 11 Dec 2015 18:27:15 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Microsoft-MacOutlook/14.4.8.150116 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [96.114.156.9] Content-Type: multipart/alternative; boundary="_000_D29040771B62Cjohnrushfordcablecomcastcom_" MIME-Version: 1.0 X-CFilter-Loop: Forward X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupkleLIzCtJLcpLzFFi42JJKJozWbdVNDvMoPGtjsWNxvnMFp+nbGRx YPL48beBxeP51n9sAUxRXDYpqTmZZalF+nYJXBk9646zF6ypqmif+oepgXFeRhcjJ4eEgIlE 34WLrBC2mMSFe+vZQGwhgaVMEssehnUxcgHZhxgl2rf8Z4Zz3n38wQrhnGSUaL03nxmkhU3A QmLbzkNgo0QElCVuvH7HCGIzCzhKzPreAhYXFsiQWDRhGgtETabEg11LGSFsI4mFKzaC1bAI qEr07L7EDmLzCthInH79D8jmAFqWJ7H/jhdImFPASeLUoq1gJYxAV38/tYYJYpW4xK0n85kg vhGQWLLnPDOELSrx8vE/sPGiAnoSh2Z9ZIGI60icvf6EEcI2kNi6dB9UXEHi/b9TbBAzEyRm HtvLBHGOoMTJmU9YICGkI7H/9xGoenGJw0d2sE5glJmF5IxZSNpnIWmHiBtIvD83nxnC1pZY tvA1lK0vsfHLWcZZQB8zCzhITFmsiaxkASPHKka55PyClOTcjPzSEgNDveTEpJxUveT83OTE 4hIQvYkRmEAWTdNx38F4odf5EKMAB6MSD+8ZjuwwIdbEsuLKXGAMcjArifCWPM8KE+JNSays Si3Kjy8qzUktPsQozcGiJM5bG+ofJiSQnliSmp2aWpBaBJNl4uCUamBctjly5cMCtaT06sKe 56m9l293H+9M5lh4ZLFTzMTv88K/z1vx+vRJz1+SixJNiqYV3GGL2yq/Y9W/2Z82n9i83ea7 0BN7zvIrYZflZsXnLzqVF7hI7MCSvVsCBddkOPJMyWiRsd7C8025LeFkkOrFpGMH9qd4PXTL tEgXblfJPqyy5alAueB1JZbijERDLeai4kQAaKzskhwDAAA= --_000_D29040771B62Cjohnrushfordcablecomcastcom_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable James, I squashed in a final with these changes, you might wish to clone the branc= h again. 1. I fixed the ownership concern. It helps having someone else review y= our code, thanks very much for that! 2. I kept the round_robin flag. As the implementation differences betwe= en no round robin, strict, and hash round robin are so trivial, I didn=92t = want to write different classes to handle them and though that just switchi= ng on the flag in one class should be okay. Let me know if you still think= I should eliminate it. 3. I would like to keep ParentResult::line_number, I have found it usefu= l in debug trouble shooting. 4. I changed lookup_strategy to selection_strategy, I think that makes i= t clearer. 5. I=92d also like to keep numParents(). It=92s not obvious now but, th= at function becomes very useful later when I add back in the multi-site ori= gin and simple/dead server retry code in HttpTransact.cc. You just don=92t= see it=92s use now since I removed that functionality for this PR. 6. I made some modifications to ParentSelection to insure that the Paren= tSelectionStrategy API is used. This helped clean up some things. 7. I like the change of recordRetrySuccess() to markParentUp() and I mad= e that change. Note that the implementation between ParentRoundRobin() and= ParentConsistentHash() is slightly different. ParentRoundRobin has one pa= rents array and ParentConsistentHash has two parent arrays (primary and sec= ondary hash lists). 8. I=92m not sure I follow the FindParent change, lets discuss more. Thanks -- John J. Rushford IPCDN Engineering 1400 Wewatta Street, Denver Colorado 80202 John_Rushford@cable.comcast.com From: James Peach > Reply-To: apache/trafficserver > Date: Wednesday, December 9, 2015 at 11:00 PM To: apache/trafficserver > Cc: John Rushford > Subject: Re: [trafficserver] TS-3418: Second hash ring for consistently has= hed parent selection (#359) Thanks @jrushf1239k. Now that we have a pre= tty clean commit on this branch, I think that it is OK to make any addition= al changes in new commits. I think the big thing that I'm concerned about in this patch is the ownersh= ip model. AFAICT, the ParentRecord object owns a ParentSelectionStrategy (I= don't see where this is deleted), but the ParentSelectionStrategy is passe= d and actually owns the pointer to its own parent which it deletes in its d= estructor. We should try to avoid parricide. A better pattern, if you can manage it, is to pass in all the information t= hat the ParentSelectionStrategy needs, rather than setting member variables= . So in the strategy construction, just keep what you need from the ParentR= ecord (probably pointers to the pRecord list). It looks like you started do= wn this path in ParentConsistentHash, so maybe you can take that further. I think you can remove ParentRecord::round_robin because it is implied by t= he strategy. We would just have additional strategies ParentStrictRoundRobi= n, ParentHashRoundRobin and ParentNoRoundRobin. It's fine for them all to l= ive in ParentRoundRobin.{c,hh}. I think you can remove ParentResult::line_number. ParentRecord::lookup_strategy should be named selection_strategy to match P= arentSelectionStrategy. I don't think you need ParentSelectionStrategy::numParents() since it is on= ly ever used for assertions. Try to just eliminate that. I think we can find better names for the ParentSelectionStrategy API. looku= pParent should be selectParent, since it is making a selection from a match= . I was going to suggest that recordRetrySuccess should be called markParen= tUp for symmetry, but then I noticed that its implementation is the same fo= r both strategies. Can we just remove it? In email I mentioned removing extApiRecord. I see now that you have preserv= ed that from the current code, so don't worry about trying to remove it. In the original code, ParentRecord had a FindParent() member function. I wo= uld prefer to keep that rather than having the caller traverse ParentRecord= ::lookup_strategy itself. It just hides the implementation a bit more nicel= y. Note that FindParent() used to take a ParentConfigParams. Since the lookup = policy is now spread over the ParentRecord and the the config_params struct= ures, it would be cleaner to condense all this into a ParentSelectionPolicy= object that is passed into FindParent. I think this would help you separat= e the ParentSelectionStrategy from owning a ParentRecord pointer too, since= state that the strategy looks at would now be passed in. =97 Reply to this email directly or view it on GitHub. --_000_D29040771B62Cjohnrushfordcablecomcastcom_--