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

[Feature] Identity carry over part 6 / Configure Identity from constuctor #194

Merged
merged 88 commits into from
Dec 19, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
88 commits
Select commit Hold shift + click to select a range
8337973
Refactor rbtree val to a comparable key
Nov 24, 2016
0876074
refactor str to a generic value to store in the tree
Nov 24, 2016
1660c01
implement walk function on rbtree for iterating the tree in order
Nov 28, 2016
97741e9
Define membership and let swim implement it.
Nov 24, 2016
03527bc
Use Reachable instead of Pingable
Nov 28, 2016
58a104c
change naming of receiver for Compare to satisfy linter
Nov 29, 2016
4838ed2
add tests for walking the tree.
Nov 29, 2016
f9c9eb7
let search return nil when items are not found
Nov 29, 2016
bf23425
more documentation refactoring.
Nov 29, 2016
20a4910
casing change for replicaPoint
Nov 29, 2016
5e842da
Merge remote-tracking branch 'origin/dev' into feature/rbtree-absraction
Nov 29, 2016
23fca53
Merge branch 'feature/rbtree-absraction' into feature/membership-package
Nov 29, 2016
65e41f9
prepare hashring to work on membership.Member instead of string.
Nov 28, 2016
735ca7f
start with changed set to false
Nov 28, 2016
a1de8bf
Implement identity on swim and ringpop via labels.
Nov 28, 2016
39d2245
expose identity on the Member.
Nov 28, 2016
19ad1a0
Use identity for points in the ring and make sure an updated identity…
Nov 28, 2016
92cde55
Use replica point with more information on it for future.
Nov 28, 2016
6eca742
abstract checksums into multiple named checksums.
Nov 28, 2016
ae69665
Add replica hashring checksummed.
Nov 28, 2016
e99f2d7
Add ring checksummer that is compatible with current released version.
Nov 28, 2016
ef8a2e4
Use tree walker to calculate checksums.
Nov 28, 2016
3c4c437
removed the legacy checksum from the map and call it legacy explicitly.
Nov 28, 2016
b5b7a47
emit named checksum periodically
Nov 28, 2016
3901340
fix low hanging fruits after feedback.
Nov 30, 2016
68566d8
Fix linting issues and non-needed export.
Nov 30, 2016
b204d2b
refactor postLocalUpdate to an updateLocalMember function that takes …
Nov 30, 2016
28c26a5
Add tests for swim memberlist emitting membership events on updates
Nov 30, 2016
7d33825
Revisit the documentation of the Member interface.
Nov 30, 2016
b3a5a9e
Add extra documentation to the Membership Event tests
Nov 30, 2016
dcc597a
Add tests for remote and local Label changes and membership.ChangeEve…
Nov 30, 2016
b2afa96
Merge branch 'feature/membership-package' into feature/extensible-has…
Nov 30, 2016
33b85a2
make tests working.
Dec 1, 2016
8362478
add missing file for tests
Dec 1, 2016
f2b8ff5
inline adding of replica points while adding server
Dec 1, 2016
0295cb9
rearrange imports
Dec 1, 2016
d5b1490
rename addresses to members in test.
Dec 1, 2016
391bf3a
Naming and consolidation.
Dec 1, 2016
43fd997
remove needles AddRemoveMembers function
Dec 1, 2016
4310b0d
add test for ProcessMembershipChanges
Dec 2, 2016
7cf7735
Merge branch 'feature/extensible-hashring' into feature/identity-carr…
Dec 2, 2016
5dec2b3
make tests work after merge
Dec 2, 2016
2ef6d5f
fix issues found by linter.
Dec 2, 2016
1af053a
Merge branch 'feature/identity-carry-over' into feature/ring-multi-ch…
Dec 2, 2016
68f0b86
Add documentation to make 'make lint' happy
Dec 2, 2016
6ad89c0
refactor in hashring
Dec 2, 2016
3f059b6
Add tests
Dec 5, 2016
b3ed09f
Added ServersUpdated to RingChangedEvent
Dec 5, 2016
730abc4
Merge branch 'feature/identity-carry-over' into feature/ring-multi-ch…
Dec 5, 2016
265c6c2
Fixes after merge
Dec 5, 2016
a047ef8
typo’s
Dec 5, 2016
399aa96
also log checksums
Dec 5, 2016
6c030dc
Add checksums to RingChecksumEvent
Dec 5, 2016
938be77
Add checksum tests
Dec 6, 2016
273d946
Add test
Dec 6, 2016
e4926c9
fix import
Dec 6, 2016
49b5026
switch it-test branch
Dec 6, 2016
fd9fd37
Identity -> Address
Dec 8, 2016
2f62bba
Support labels before bootstrap
Dec 14, 2016
80205d6
Add identity-option
Dec 14, 2016
8373307
Switch to feature/identity branch of ringpop-common
Dec 14, 2016
c52f066
Remove old api
Dec 14, 2016
38aa51e
improve documentation
Dec 14, 2016
30e3c60
enable identity integration tests
Dec 14, 2016
bfb5e86
Rename walk
Dec 14, 2016
e4d8dd1
Feedback
Dec 14, 2016
3d0d532
feedback
Dec 15, 2016
31e630b
Renamed checksum -> checksummer
Dec 15, 2016
101e4f8
switch it-tests back to master after merg
Dec 15, 2016
2921874
move error
Dec 15, 2016
1259782
switch back to ringpop-common#master
Dec 15, 2016
2269706
feedback
Dec 15, 2016
fb81d89
Merge branch 'feature/rbtree-absraction' into feature/membership-package
Dec 15, 2016
60515d0
Merge branch 'feature/membership-package' into feature/extensible-has…
Dec 15, 2016
7c85ff4
Merge branch 'feature/extensible-hashring' into feature/identity-carr…
Dec 15, 2016
74de8fe
Merge branch 'feature/identity-carry-over' into feature/ring-multi-ch…
Dec 15, 2016
3cf4b5c
Merge branch 'feature/ring-multi-checksum' into feature/refactor-iden…
Dec 15, 2016
fac34e4
fix ringpop-common
Dec 15, 2016
ecf4f8a
Merge branch 'feature/ring-multi-checksum' into feature/refactor-iden…
Dec 15, 2016
a4d8488
rename traverse function
Dec 16, 2016
a0468ac
Merge branch 'feature/rbtree-absraction' into feature/membership-package
Dec 16, 2016
d721c7e
Merge branch 'feature/membership-package' into feature/extensible-has…
Dec 16, 2016
0b6f6e8
Merge branch 'feature/extensible-hashring' into feature/identity-carr…
Dec 16, 2016
3e015c5
Merge branch 'feature/identity-carry-over' into feature/ring-multi-ch…
Dec 16, 2016
c7ef3e7
fix after merge
Dec 16, 2016
388a5f8
Merge branch 'feature/ring-multi-checksum' into feature/refactor-iden…
Dec 16, 2016
21611c5
Merge remote-tracking branch 'origin/dev' into feature/refactor-ident…
Dec 19, 2016
5c97e4a
Merge remote-tracking branch 'origin/dev' into feature/refactor-ident…
Dec 19, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.PHONY: clean clean-mocks coveralls testpop lint mocks out setup test test-integration test-unit test-race
.PHONY: clean clean-common clean-mocks coveralls testpop lint mocks out setup test test-integration test-unit test-race

SHELL = /bin/bash

Expand All @@ -24,6 +24,9 @@ out: test
clean:
rm -f testpop

clean-common:
rm -rf test/ringpop-common

clean-mocks:
rm -f test/mocks/*.go forward/mock_*.go
rm -rf test/thrift/pingpong/
Expand Down
18 changes: 18 additions & 0 deletions options.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ import (
log "github.com/uber-common/bark"
"github.com/uber/ringpop-go/hashring"
"github.com/uber/ringpop-go/logging"
"github.com/uber/ringpop-go/membership"
"github.com/uber/ringpop-go/shared"
"github.com/uber/ringpop-go/swim"
"github.com/uber/ringpop-go/util"
)

type configuration struct {
Expand All @@ -52,6 +54,9 @@ type configuration struct {
// number of labels and the size of key and value can be configured.
LabelLimits swim.LabelOptions

// InitialLabels configures the initial labels.
InitialLabels swim.LabelMap

// SelfEvict holds the settings with regards to self eviction
SelfEvict swim.SelfEvictOptions
}
Expand Down Expand Up @@ -173,6 +178,19 @@ func Statter(s log.StatsReporter) Option {
}
}

// Identity is used to specify an identity as this Ringpop instance's identity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Help text could be better here.

E.g.:

// Identity can be used to specify a custom string as the unique identifier for
// this node. The identity should be unique amongst other Ringpop instances; it
// is used in the hashring.
//
// By default, the hostport/address of the node is used as the identity in the
// hashring. An error is thrown if a hostport is manually specified using this
// option, as this would lead to unexpected behaviour. If you want to override
// the node's listening address, use the `Address` option.

// Since a ringpop instance is by default identified by its hostport, it's not allowed to manually specify a
// hostport as its identity.
func Identity(identity string) Option {
return func(r *Ringpop) error {
if util.HostportPattern.MatchString(identity) {
return errors.New("a hostport is not valid as an identity")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe define an error at the top that is exported and returned here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was thinking the same but all other errors in this file are created inline. I chose to follow the code style (of this file).

}
r.config.InitialLabels[membership.IdentityLabelKey] = identity
return nil
}
}

// Address is used to specify a static hostport string as this Ringpop
// instance's address.
//
Expand Down
26 changes: 26 additions & 0 deletions options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/stretchr/testify/suite"
"github.com/uber/ringpop-go/hashring"
"github.com/uber/ringpop-go/logging"
"github.com/uber/ringpop-go/membership"
"github.com/uber/ringpop-go/swim"
"github.com/uber/ringpop-go/test/mocks"
"github.com/uber/tchannel-go"
Expand Down Expand Up @@ -183,6 +184,31 @@ func (s *RingpopOptionsTestSuite) TestMissingAddressResolver() {
s.Error(err)
}

func (s *RingpopOptionsTestSuite) TestIdentity() {
rp, err := New("test", Channel(s.channel), Identity("identity"))
s.Require().NotNil(rp)
s.Require().NoError(err)

identity, has := rp.config.InitialLabels[membership.IdentityLabelKey]
s.Equal(true, has, "Identity label set")
s.Equal("identity", identity)
}

func (s *RingpopOptionsTestSuite) TestDefaultIdentity() {
rp, err := New("test", Channel(s.channel))
s.Require().NotNil(rp)
s.Require().NoError(err)

_, has := rp.config.InitialLabels[membership.IdentityLabelKey]
s.Equal(false, has, "Identity label not set")
}

func (s *RingpopOptionsTestSuite) TestHostPortIdentity() {
rp, err := New("test", Channel(s.channel), Identity("127.0.0.1:1234"))
s.Nil(rp)
s.Error(err)
}

// TestClockNil confirms that nil clock option returns an error.
func (s *RingpopOptionsTestSuite) TestClockNil() {
rp, err := New("test", Clock(nil))
Expand Down
16 changes: 3 additions & 13 deletions ringpop.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ func New(app string, opts ...Option) (*Ringpop, error) {

ringpop := &Ringpop{
config: &configuration{
App: app,
App: app,
InitialLabels: make(swim.LabelMap),
},
logger: logging.Logger("ringpop"),
}
Expand Down Expand Up @@ -193,6 +194,7 @@ func (rp *Ringpop) init() error {
StateTimeouts: rp.config.StateTimeouts,
Clock: rp.clock,
LabelLimits: rp.config.LabelLimits,
InitialLabels: rp.config.InitialLabels,
SelfEvict: rp.config.SelfEvict,
})
rp.node.AddListener(rp)
Expand Down Expand Up @@ -797,18 +799,6 @@ func (rp *Ringpop) Labels() (*swim.NodeLabels, error) {
return rp.node.Labels(), nil
}

// SetIdentity changes the identity for this process. The identity is used by
// all members of the membership to calculate the position of this node in the
// hashring.
func (rp *Ringpop) SetIdentity(identity string) error {
if !rp.Ready() {
return ErrNotBootstrapped
}

rp.node.SetIdentity(identity)
return nil
}

// SerializeThrift takes a thrift struct and returns the serialized bytes
// of that struct using the thrift binary protocol. This is a temporary
// measure before frames can forwarded directly past the endpoint to the proper
Expand Down
8 changes: 8 additions & 0 deletions ringpop_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,14 @@ func (s *RingpopTestSuite) TestStartTimersIdempotance() {
s.ringpop.stopTimers()
}

func (s *RingpopTestSuite) TestInitialLabels() {
s.ringpop.config.InitialLabels["key"] = "value"
s.ringpop.init()
value, has := s.ringpop.node.Labels().Get("key")
s.Equal("value", value, "Label correctly set on local node")
s.True(has, "Label correctly set on local node")
}

func (s *RingpopTestSuite) TestReadyEvent() {
called := make(chan bool, 1)

Expand Down
6 changes: 6 additions & 0 deletions scripts/testpop/testpop.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ var (
faultyPeriod = flag.Int("faulty-period", 24*60*60*1000, "The lifetime of a faulty member in ms. After that the member becomes a tombstone.")
tombstonePeriod = flag.Int("tombstone-period", 5000, "The lifetime of a tombstone member in ms. After that the member is removed from the membership.")

identity = flag.String("identity", "", "specify an identity")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should at least say: specify an identity for this node in the hashring. Otherwise no point in even having the help text. The fact that it's setting something called identity is already obvious from the name.


hostportPattern = regexp.MustCompile(`^(\d+.\d+.\d+.\d+):\d+$`)
)

Expand Down Expand Up @@ -78,6 +80,10 @@ func main() {
ringpop.TombstonePeriod(time.Duration(*tombstonePeriod) * time.Millisecond),
}

if *identity != "" {
options = append(options, ringpop.Identity(*identity))
}

if *statsUDP != "" && *statsFile != "" {
log.Fatalf("-stats-udp and stats-file are mutually exclusive.")
}
Expand Down
10 changes: 6 additions & 4 deletions swim/join_sender.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,11 @@ var errJoinTimeout = errors.New("join timed out")

// A joinRequest is used to request a join to a remote node
type joinRequest struct {
App string `json:"app"`
Source string `json:"source"`
Incarnation int64 `json:"incarnationNumber"`
Timeout time.Duration `json:"timeout"`
App string `json:"app"`
Source string `json:"source"`
Incarnation int64 `json:"incarnationNumber"`
Timeout time.Duration `json:"timeout"`
Labels map[string]string `json:"labels,omitempty"`
}

// joinOpts are opts to perform a join with
Expand Down Expand Up @@ -446,6 +447,7 @@ func sendJoinRequest(node *Node, target string, timeout time.Duration) (*joinRes
Source: node.address,
Incarnation: node.Incarnation(),
Timeout: timeout,
Labels: node.Labels().AsMap(),
}
res := &joinResponse{}

Expand Down
3 changes: 2 additions & 1 deletion swim/memberlist.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type memberlist struct {
}

// newMemberlist returns a new member list
func newMemberlist(n *Node) *memberlist {
func newMemberlist(n *Node, initialLabels LabelMap) *memberlist {
m := &memberlist{
node: n,
logger: logging.Logger("membership").WithField("local", n.address),
Expand All @@ -71,6 +71,7 @@ func newMemberlist(n *Node) *memberlist {
Address: n.Address(),
Incarnation: nowInMillis(n.clock),
Status: Alive,
Labels: initialLabels,
},
}

Expand Down
5 changes: 3 additions & 2 deletions swim/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ type Options struct {
PartitionHealPeriod time.Duration
PartitionHealBaseProbabillity float64

LabelLimits LabelOptions
LabelLimits LabelOptions
InitialLabels LabelMap

Clock clock.Clock

Expand Down Expand Up @@ -250,7 +251,7 @@ func NewNode(app, address string, channel shared.SubChannel, opts *Options) *Nod

node.labelLimits = opts.LabelLimits

node.memberlist = newMemberlist(node)
node.memberlist = newMemberlist(node, opts.InitialLabels)
node.memberiter = newMemberlistIter(node.memberlist)
node.stateTransitions = newStateTransitions(node, opts.StateTimeouts)

Expand Down
2 changes: 1 addition & 1 deletion test/lib.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

declare project_root="${0%/*}/.."
declare ringpop_common_dir="${0%/*}/ringpop-common"
declare ringpop_common_branch="menno/allow-multi-checksum"
declare ringpop_common_branch="feature/identity"
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets make sure to change this to master before landing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!


#
# Clones or updates the ringpop-common repository.
Expand Down