trafficserver-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rushford, John" <John_Rushf...@cable.comcast.com>
Subject Re: [trafficserver] TS-3418: Second hash ring for consistently hashed parent selection (#359)
Date Fri, 11 Dec 2015 18:27:15 GMT
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_Rushford@cable.comcast.com











From: James Peach <notifications@github.com<mailto:notifications@github.com>>
Reply-To: apache/trafficserver <reply+0045726ef5aeadcb64197c9f8b17607e9ab2ed2249aa3f7792cf000000011280d70e92a169ce0726e125@reply.github.com<mailto:reply+0045726ef5aeadcb64197c9f8b17607e9ab2ed2249aa3f7792cf000000011280d70e92a169ce0726e125@reply.github.com>>
Date: Wednesday, December 9, 2015 at 11:00 PM
To: apache/trafficserver <trafficserver@noreply.github.com<mailto:trafficserver@noreply.github.com>>
Cc: John Rushford <john_rushford@cable.comcast.com<mailto:john_rushford@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>.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message