trafficserver-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jpeach <...@git.apache.org>
Subject [GitHub] trafficserver pull request: TS-3418: Second hash ring for consiste...
Date Thu, 10 Dec 2015 06:00:43 GMT
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message