[
https://issues.apache.org/jira/browse/TS-3418?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15050134#comment-15050134
]
ASF GitHub Bot commented on TS-3418:
------------------------------------
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/359#issuecomment-163500832
Thanks @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.
> Second hash ring for consistently hashed parent selection
> ----------------------------------------------------------
>
> Key: TS-3418
> URL: https://issues.apache.org/jira/browse/TS-3418
> Project: Traffic Server
> Issue Type: New Feature
> Components: Parent Proxy
> Reporter: Leif Hedstrom
> Assignee: John Rushford
> Fix For: 6.1.0
>
> Time Spent: 336h
> Remaining Estimate: 0h
>
> It would be incredibly useful if we allowed for an (optional) second hash
> ring in the consistent hashing in parent selection. Imagine a setup where you
> have two set of parent proxies. A child would prefer to always use a parent
> <n> in ring <A> for a set of URLs, <X>. In the case of parent <n> not being
> available, instead of rehashing <X> to the surviving members of ring <A>, we
> could now hash the URLs to parent <m> in ring <B>. Upon failure there, we'd
> then go back and rehash on the primary ring again (<A>).
> This sounds complicated, but is simple in principle. Instead of immediately
> rehashing content upon a parent failure, we have a backup pool (potentially
> remote) of parents, that are likely to have the content. The idea is to
> minimize origin server traffic at all cost.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
