James, I squashed in a final with these changes, you might wish to clone the branch again.
1. I fixed the ownership concern. It helps having someone else review your code, thanks very much for that! 2. I kept the round_robin flag. As the implementation differences between no round robin, strict, and hash round robin are so trivial, I didn’t want to write different classes to handle them and though that just switching 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 useful in debug trouble shooting. 4. I changed lookup_strategy to selection_strategy, I think that makes it clearer. 5. I’d also like to keep numParents(). It’s not obvious now but, that function becomes very useful later when I add back in the multi-site origin and simple/dead server retry code in HttpTransact.cc. You just don’t see it’s use now since I removed that functionality for this PR. 6. I made some modifications to ParentSelection to insure that the ParentSelectionStrategy API is used. This helped clean up some things. 7. I like the change of recordRetrySuccess() to markParentUp() and I made that change. Note that the implementation between ParentRoundRobin() and ParentConsistentHash() is slightly different. ParentRoundRobin has one parents array and ParentConsistentHash has two parent arrays (primary and secondary hash lists). 8. I’m not sure I follow the FindParent change, lets discuss more. Thanks -- John J. Rushford IPCDN Engineering 1400 Wewatta Street, Denver Colorado 80202 john_rushf...@cable.comcast.com From: James Peach <notificati...@github.com<mailto:notificati...@github.com>> Reply-To: apache/trafficserver <reply+0045726ef5aeadcb64197c9f8b17607e9ab2ed2249aa3f7792cf000000011280d70e92a169ce0726e...@reply.github.com<mailto:reply+0045726ef5aeadcb64197c9f8b17607e9ab2ed2249aa3f7792cf000000011280d70e92a169ce0726e...@reply.github.com>> Date: Wednesday, December 9, 2015 at 11:00 PM To: apache/trafficserver <trafficser...@noreply.github.com<mailto:trafficser...@noreply.github.com>> Cc: John Rushford <john_rushf...@cable.comcast.com<mailto:john_rushf...@cable.comcast.com>> Subject: Re: [trafficserver] TS-3418: Second hash ring for consistently hashed parent selection (#359) Thanks @jrushf1239k<https://github.com/jrushf1239k>. Now that we have a pretty clean commit on this branch, I think that it is OK to make any additional changes in new commits. I think the big thing that I'm concerned about in this patch is the ownership model. AFAICT, the ParentRecord object owns a ParentSelectionStrategy (I don't see where this is deleted), but the ParentSelectionStrategy is passed and actually owns the pointer to its own parent which it deletes in its destructor. We should try to avoid parricide. A better pattern, if you can manage it, is to pass in all the information that the ParentSelectionStrategy needs, rather than setting member variables. So in the strategy construction, just keep what you need from the ParentRecord (probably pointers to the pRecord list). It looks like you started down this path in ParentConsistentHash, so maybe you can take that further. I think you can remove ParentRecord::round_robin because it is implied by the strategy. We would just have additional strategies ParentStrictRoundRobin, ParentHashRoundRobin and ParentNoRoundRobin. It's fine for them all to live in ParentRoundRobin.{c,hh}. I think you can remove ParentResult::line_number. ParentRecord::lookup_strategy should be named selection_strategy to match ParentSelectionStrategy. I don't think you need ParentSelectionStrategy::numParents() since it is only ever used for assertions. Try to just eliminate that. I think we can find better names for the ParentSelectionStrategy API. lookupParent should be selectParent, since it is making a selection from a match. I was going to suggest that recordRetrySuccess should be called markParentUp for symmetry, but then I noticed that its implementation is the same for both strategies. Can we just remove it? In email I mentioned removing extApiRecord. I see now that you have preserved 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 would prefer to keep that rather than having the caller traverse ParentRecord::lookup_strategy itself. It just hides the implementation a bit more nicely. Note that FindParent() used to take a ParentConfigParams. Since the lookup policy is now spread over the ParentRecord and the the config_params structures, it would be cleaner to condense all this into a ParentSelectionPolicy object that is passed into FindParent. I think this would help you separate the ParentSelectionStrategy from owning a ParentRecord pointer too, since state that the strategy looks at would now be passed in. — Reply to this email directly or view it on GitHub<https://github.com/apache/trafficserver/pull/359#issuecomment-163500832>.
