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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
