Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for upstream groups #179

Merged
merged 6 commits into from
May 6, 2016
Merged

add support for upstream groups #179

merged 6 commits into from
May 6, 2016

Conversation

tpetr
Copy link
Contributor

@tpetr tpetr commented May 4, 2016

  • Adds optional group field to UpstreamInfo class (defaults to DEFAULT if absent)
  • Adds additional upstreamGroups field to ServiceContext class (the handlebars templates use this). Upstreams without a group value are added to the default group.
  • The original upstreams list in ServiceContext is not affected -- this will contain all upstreams.

@ssalinas

@ssalinas
Copy link
Member

ssalinas commented May 5, 2016

@tpetr meant to comment yesterday, two things:

  • won't need a migration, since it's at the end of the upstream string and we use original path for deletions, should be fine
  • We should probably still have the validation in there so the group value doesn't mess up our zk path. Why not do what we did for rackId and requestId where we default it to empty in the constructor? or could have an actual default there if that makes more sense

@tpetr
Copy link
Contributor Author

tpetr commented May 5, 2016

Thanks for the feedback, I have an internal TODO to re-add validation on the group field.

@ssalinas ssalinas added the qa label May 5, 2016
public boolean sameAs(UpstreamInfo that) {
return upstream.equals(that.upstream) && group.equals(that.group);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is mostly just stylistic, but I'd personally make this a static method named something like UpstreamInfo.upstreamsAndGroupMatch(UpstreamInfo a, UpstreamInfo b)

@ssalinas ssalinas merged commit 72ae63e into master May 6, 2016
@ssalinas ssalinas deleted the upstream-groups branch May 6, 2016 15:26
@ssalinas ssalinas removed the staging label May 6, 2016
@ssalinas ssalinas modified the milestone: 0.4.0 May 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants