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.

Sign up for GitHub

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

Jump to bottom

attributes: add Equal method; resolver: add AddressMap and State.BalancerAttributes #4855

Merged
merged 7 commits into from
Oct 15, 2021
Merged

attributes: add Equal method; resolver: add AddressMap and State.BalancerAttributes #4855

merged 7 commits into from
Oct 15, 2021

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Oct 8, 2021 •
edited
Loading

Hint for easier reviewing: review in the following order:

  1. attributes.go / attributes_test.go
  2. resolver.go / map.go / map_test.go
  3. roundrobin.go / roundrobin_test.go
  4. everything else

RELEASE NOTES:

  • attributes: add Equal method; resolver: add AddressMap and State.BalancerAttributes

@dfawley dfawley added the Type: API Change Breaking API changes (experimental APIs only!) label Oct 8, 2021
@dfawley dfawley added this to the 1.42 Release milestone Oct 8, 2021
@dfawley dfawley requested a review from menghanl October 8, 2021 03:08
menghanl
menghanl reviewed Oct 13, 2021
View reviewed changes
Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

Initial comments for attributes.go

attributes/attributes.go Outdated
a.m[kvs[i*2]] = kvs[i*2+1]
}
return a
// Value must be implemented by all values stored in Attributes. It allows
Copy link
Contributor

Choose a reason for hiding this comment

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

Or make it optional? If not implemented, values are always considered not-equal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Update the comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Should I rename this to Equal (or remove it)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing (and adding comment to attributes) SGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

Done; PTAL.

attributes/attributes.go Outdated Show resolved Hide resolved
attributes/attributes.go Outdated Show resolved Hide resolved
attributes/attributes.go Outdated Show resolved Hide resolved
menghanl
menghanl suggested changes Oct 14, 2021
View reviewed changes
attributes/attributes.go Outdated
a.m[kvs[i*2]] = kvs[i*2+1]
}
return a
// Value must be implemented by all values stored in Attributes. It allows
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the comment.

attributes/attributes_test.go Show resolved Hide resolved
resolver/resolver.go Outdated Show resolved Hide resolved
resolver/map.go Show resolved Hide resolved
balancer/roundrobin/roundrobin_test.go Show resolved Hide resolved
menghanl
menghanl suggested changes Oct 14, 2021
View reviewed changes
internal/credentials/xds/handshake_info.go Outdated Show resolved Hide resolved
internal/resolver/config_selector.go Outdated Show resolved Hide resolved
xds/internal/internal.go Outdated Show resolved Hide resolved
internal/hierarchy/hierarchy.go Outdated Show resolved Hide resolved
balancer/grpclb/state/state.go Show resolved Hide resolved
xds/internal/xdsclient/attributes.go Outdated Show resolved Hide resolved
@menghanl menghanl assigned dfawley and unassigned menghanl Oct 14, 2021
@dfawley dfawley assigned menghanl and unassigned dfawley Oct 14, 2021
menghanl
menghanl approved these changes Oct 14, 2021
View reviewed changes
balancer/grpclb/state/state.go Show resolved Hide resolved
@menghanl menghanl assigned dfawley and unassigned menghanl Oct 15, 2021
@dfawley dfawley changed the title attributes: add Value interface with IsEqual; add resolver.AddressMap attributes: add Equal method; resolver: add AddressMap and State.BalancerAttributes Oct 15, 2021
@dfawley dfawley added Type: Feature New features or improvements in behavior and removed Type: API Change Breaking API changes (experimental APIs only!) labels Oct 15, 2021
@dfawley dfawley merged commit 36d8757 into grpc:master Oct 15, 2021
@dfawley dfawley deleted the attributed branch October 15, 2021 17:39
@benpbrown benpbrown mentioned this pull request Jan 4, 2022
Closed
@dfawley dfawley mentioned this pull request Mar 30, 2022
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@menghanl menghanl menghanl approved these changes

Assignees

@dfawley dfawley

Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
1.42 Release
Development

Successfully merging this pull request may close these issues.
2 participants
@dfawley @menghanl