Issue 35790047: mongo replicaset configuration
Descriptionmongo replicaset configuration
These changes encompass an API for setting up and controlling a mongo replicaset directly through mgo. In order to facilitate testing, the code under testing to set up a test mongo server was refactored to allow it to be used for multiple mongo servers (previously it used a lot of package global variables).
https://code.launchpad.net/~natefinch/juju-core/mongo-ha/+merge/197916
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : mongo replicaset configuration #
Total comments: 23
Patch Set 3 : mongo replicaset configuration #
Total comments: 14
Patch Set 4 : mongo replicaset configuration #MessagesTotal messages: 13
Please take a look.
Sign in to reply to this message.
Don't review this yet, I am getting some sporadic test failures that need to be looked into.
Sign in to reply to this message.
On 2013/12/05 16:27:35, nate.finch wrote: > Don't review this yet, I am getting some sporadic test failures that need to be > looked into. OK, I'll WIP it. If you can extract the global-killing and propose that on its own, it'd be cool.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
LGTM modulo the below. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... File environs/replicaset/replicaset.go (right): https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset.go:26: // with MemberDefaults instead. MemberDefaults does not seem to exist. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset.go:36: // Arbiter holds whether the member is an arbiter only, // defaulting to false if nil. (and likewise for other pointer-valued fields - I don't think we should need to go to the docs to work out what the defaults are) https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset.go:138: // that already existed Doesn't seem too ugly to me. // Assign new ids to members that did not previously exist. would perhaps be a sufficient comment though. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset.go:217: // the replicaset s/replicaset/replica set/ https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset.go:289: func (state MemberState) String() string { I'd probably do: var memberStateStrings []string{ StartupState: "STARTUP", PrimaryState: "PRIMARY", .... } func (state MemberState) String() string { if state < 0 || state >= len(memberStateStrings) { return "INVALID_MEMBER_STATE" } return memberStateStrings[state] } https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... File environs/replicaset/replicaset_test.go (right): https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:22: // do all this stuff here, since we don't want to have to redo it for each test Why not do it in SetUpSuite instead? https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:59: inst := &coretesting.MgoInstance{} inst := &coretesting.MgoInstance{ Params: []string{"--replSet", name}, } ? https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:105: // Add should automatically skip root, so test that I don't understand this comment. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:120: err = Add(session, members[0:5]...) s/members[0:5]/members/ ? (Why wouldn't len(members) be 5?) BTW I'd be surprised if this works. Don't we need to add servers slowly, waiting for existing ones to come up? https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:176: exp := IsMasterResults{ s/exp/expect/ ? https://codereview.appspot.com/35790047/diff/20001/testing/mgo.go File testing/mgo.go (right): https://codereview.appspot.com/35790047/diff/20001/testing/mgo.go#newcode43 testing/mgo.go:43: // Server holds the running MongoDB command. I'm not sure about exporting all these fields. A suggestion below.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... File environs/replicaset/replicaset.go (right): https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset.go:26: // with MemberDefaults instead. On 2013/12/11 12:05:07, rog wrote: > MemberDefaults does not seem to exist. Oh yeah, oops, this is a holdover from the original way I had written it. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset.go:36: // Arbiter holds whether the member is an arbiter only, On 2013/12/11 12:05:07, rog wrote: > // defaulting to false if nil. > > (and likewise for other pointer-valued fields - I don't > think we should need to go to the docs to work out > what the defaults are) Ahh, yes, definitely. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset.go:138: // that already existed On 2013/12/11 12:05:07, rog wrote: > Doesn't seem too ugly to me. > > // Assign new ids to members that did not previously exist. > > would perhaps be a sufficient comment though. That's fine. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset.go:217: // the replicaset On 2013/12/11 12:05:07, rog wrote: > s/replicaset/replica set/ Done. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset.go:289: func (state MemberState) String() string { On 2013/12/11 12:05:07, rog wrote: > I'd probably do: > > var memberStateStrings []string{ > StartupState: "STARTUP", > PrimaryState: "PRIMARY", > .... > } > > func (state MemberState) String() string { > if state < 0 || state >= len(memberStateStrings) { > return "INVALID_MEMBER_STATE" > } > return memberStateStrings[state] > } Wow, I didn't even know you could do that. Slick. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... File environs/replicaset/replicaset_test.go (right): https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:22: // do all this stuff here, since we don't want to have to redo it for each test On 2013/12/11 12:05:07, rog wrote: > Why not do it in SetUpSuite instead? Hmm, good point. That's exactly where it belongs, I just wasn't thinking. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:59: inst := &coretesting.MgoInstance{} On 2013/12/11 12:05:07, rog wrote: > inst := &coretesting.MgoInstance{ > Params: []string{"--replSet", name}, > } > > ? Yeah, funny, not sure why I didn't do that to start with. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:105: // Add should automatically skip root, so test that On 2013/12/11 12:05:07, rog wrote: > I don't understand this comment. I'll clarify. basically just means you should be able to add a machine that's already in the replica set and not have it break anything. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:120: err = Add(session, members[0:5]...) On 2013/12/11 12:05:07, rog wrote: > s/members[0:5]/members/ ? > (Why wouldn't len(members) be 5?) > > BTW I'd be surprised if this works. Don't we need to add servers slowly, waiting > for existing ones to come up? Ahh, sorry, that was carry over from earlier code that was only adding some of the members and then adding more later. I can fix it. Also, it works as long as a majority of the replicas are up and running. They don't all have to be synced, that can happen later. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:176: exp := IsMasterResults{ On 2013/12/11 12:05:07, rog wrote: > s/exp/expect/ ? Sure. Since tests are so often expected / obtained, I tend to abbreviate, but it doesn't hurt to have it a little more clear.
Sign in to reply to this message.
LGTM modulo the below. https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... File environs/replicaset/replicaset_test.go (right): https://codereview.appspot.com/35790047/diff/20001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:176: exp := IsMasterResults{ On 2013/12/12 11:32:06, nate.finch wrote: > On 2013/12/11 12:05:07, rog wrote: > > s/exp/expect/ ? > > Sure. Since tests are so often expected / obtained, I tend to abbreviate, but it > doesn't hurt to have it a little more clear. The suggestion was really just for consistency - I have nothing against abbreviations in general if they're commonly used, but we don't use "exp" for "expect" much in this code base. I still think it's worth doing. https://codereview.appspot.com/35790047/diff/20001/testing/mgo.go File testing/mgo.go (right): https://codereview.appspot.com/35790047/diff/20001/testing/mgo.go#newcode43 testing/mgo.go:43: // Server holds the running MongoDB command. On 2013/12/11 12:05:07, rog wrote: > I'm not sure about exporting all these fields. > A suggestion below. Oops, looks like I forgot to post the suggestion. Just wondering why we don't have something more like this, with no fields exported in MgoInstance. Then it's obvious how MgoInstance should be created. // MgoInstance returns a new running mongo server instance // with the given extra command line arguments. func NewMgoInstance(params ...string) (*MgoInstance, error) func (inst *MgoInstance) Reset() error func (inst *MgoInstance) Addr() string func (inst *MgoInstance) Port() int ... other methods. https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... File environs/replicaset/replicaset.go (right): https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset.go:35: // Value is optional, defaults to false. // This value is optional; it defaults to false. and similarly below? https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... File environs/replicaset/replicaset_test.go (right): https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:59: func() { Can we just move this out into its own function? The anonymous function is cute but not a significant win in this case IMO. https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:64: if err != nil { c.Assert(err, gc.IsNil) and similarly below. https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:78: if !reflect.DeepEqual(mems, expectedMembers) { c.Assert(mems, gc.DeepEquals, expectedMembers) https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset_test.go:82: // now add some data so we get a more real-life test perhaps this might work well in a separate function.
Sign in to reply to this message.
I didn't finish the review until I got called away, but I can give you some feedback. I'm coming to this a bit late, so don't take this as "you must rewrite everything you're doing", rather some thoughts that you can take under consideration. https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... File environs/replicaset/replicaset.go (right): https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset.go:13: // called only once for a given mongo replica set. Is this "it should be called at least once" or "it should be called only once", or ? I think it would be helpful to be clear that this is creating a new replica set with a single entry and initializing it? https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset.go:18: func Initiate(session *mgo.Session, address, name string) error { I would usually use the term "Initialize", but I guess the name is just coming from the Mongo internals? (replSetInitiate) https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset.go:31: // See http://goo.gl/VYnZ2z I'd actually rather use full URLs http://docs.mongodb.org/v2.2/reference/replica-configuration/#local.system.re... because that gives you a hint as to where you are going (and you can be sure it isn't a goatse link.) I'm also a little surprised this is pointing to v2.2 given we use 2.4 in practice. Alternatively something like named shortened links, but VYnZ2z certainly doesn't mean anything to anyone but goo.gl. https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset.go:37: Arbiter *bool `bson:"arbiterOnly,omitempty"` I'm a bit torn about *bool. I won't derail you here, but you can't just do &true, so you have to play lots of tricks when you actually want to set them. It would appear that mgo.DialInfo uses the "write it in Go convention" model. And the goal is to end up with replicaset inside mgo, right? https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset.go:42: BuildIndexes *bool `bson:"buildIndexes,omitempty"` I guess this one might be hard because you actually need it spelled that way on the wire. It feels like it would be worth having a private serialization struct that was then mapped onto the public Go-like params struct. (OmitIndexes seems a reasonable name to invert the default-false requirement) https://codereview.appspot.com/35790047/diff/40001/testing/mgo.go File testing/mgo.go (right): https://codereview.appspot.com/35790047/diff/40001/testing/mgo.go#newcode59 testing/mgo.go:59: const mgoDialTimeout = 15 * time.Second I didn't see a mention of why this had to change. Is it just the load of bringing up multiple servers?
Sign in to reply to this message.
https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... File environs/replicaset/replicaset.go (right): https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset.go:13: // called only once for a given mongo replica set. On 2013/12/12 14:17:45, jameinel wrote: > Is this "it should be called at least once" or "it should be called only once", > or ? s/only/exactly/ ? > > I think it would be helpful to be clear that this is creating a new replica set > with a single entry and initializing it? +1 https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset.go:31: // See http://goo.gl/VYnZ2z On 2013/12/12 14:17:45, jameinel wrote: > I'd actually rather use full URLs > http://docs.mongodb.org/v2.2/reference/replica-configuration/#local.system.re... > > because that gives you a hint as to where you are going (and you can be sure it > isn't a goatse link.) > > I'm also a little surprised this is pointing to v2.2 given we use 2.4 in > practice. > > Alternatively something like named shortened links, but VYnZ2z certainly doesn't > mean anything to anyone but goo.gl. I think the important thing is go along with the existing labix.org/v2/mgo conventions, which do seem to include the whole links, so +1 here. Sorry about that Nate; I think I suggested the goo.gl links originally, because that's the way goamz does it. https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... environs/replicaset/replicaset.go:42: BuildIndexes *bool `bson:"buildIndexes,omitempty"` On 2013/12/12 14:17:45, jameinel wrote: > I guess this one might be hard because you actually need it spelled that way on > the wire. > > It feels like it would be worth having a private serialization struct that was > then mapped onto the public Go-like params struct. > > (OmitIndexes seems a reasonable name to invert the default-false requirement) We've had that discussion. I agree with you (and made specific suggestions in that direction), but I think it's reasonable that Nate can choose differently here.
Sign in to reply to this message.
On 2013/12/12 14:55:41, rog wrote: > > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > environs/replicaset/replicaset.go:42: BuildIndexes *bool > `bson:"buildIndexes,omitempty"` > On 2013/12/12 14:17:45, jameinel wrote: > > It feels like it would be worth having a private serialization struct that was > > then mapped onto the public Go-like params struct. > > We've had that discussion. I agree with you (and made specific suggestions in > that direction), but I think it's reasonable that Nate can choose differently > here. So, the main problem with having non-pointer values in the Member struct is that the defaults for the types are not the same as the defaults for Mongo. For example, Votes and Priority both default to 1 in Mongo. And in fact, setting them to 0 has very important impacts on the replicaset behavior (priority 0 means the replica can never become primary, and votes 0 is obvious). The booleans could be flipped appropriately so defaulting to false means what the default in Mongo means (and have an internal serializing struct that flips them back), but that still leaves us with Votes and Priority being pointers. Also, flipping the booleans around might confuse people who are used to Mongo's values, and set them what they expect them to be in Mongo, which is actually opposite what they'd be in our code. I don't want people to make an instance of the struct and have the defaults be something completely different than the normal default state. Roger suggested having a DefaultMember public value that could be used to initialize new values to the proper defaults, but that feels clumsy and easily missed / misunderstood. Most of the time people are likely just going to be adding regular mongo servers with the normal defaults. My way is the most natural way to allow that to happen. They can just say m := Member{Address:"foobar"} and it'll be the right thing. The other popular styles of server are an arbiter and a pure backup (non-voting, 0 priority). We could make a couple helper functions to return values with the correct properties set for each of those. And then the only time anyone would need to make pointers to things themselves is when they want wacky configurations.
Sign in to reply to this message.
On 2013/12/12 14:17:45, jameinel wrote: > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > File environs/replicaset/replicaset.go (right): > > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > environs/replicaset/replicaset.go:13: // called only once for a given mongo > replica set. > Is this "it should be called at least once" or "it should be called only once", > or ? > > I think it would be helpful to be clear that this is creating a new replica set > with a single entry and initializing it? Cleaned up the comment to mention it makes a replica set with a single replica. It actually occurs to me that you could pass in a list of servers and create the replica set all at once, but I haven't implemented that, since I just wanted to get this in. > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > environs/replicaset/replicaset.go:18: func Initiate(session *mgo.Session, > address, name string) error { > I would usually use the term "Initialize", but I guess the name is just coming > from the Mongo internals? (replSetInitiate) Yeah, there's also the javascript method rs.Initiate(). Figured we might as well use their terminology. > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > environs/replicaset/replicaset.go:31: // See http://goo.gl/VYnZ2z > I'd actually rather use full URLs > http://docs.mongodb.org/v2.2/reference/replica-configuration/#local.system.re... > > because that gives you a hint as to where you are going (and you can be sure it > isn't a goatse link.) > > I'm also a little surprised this is pointing to v2.2 given we use 2.4 in > practice. > > Alternatively something like named shortened links, but VYnZ2z certainly doesn't > mean anything to anyone but goo.gl. As Roger said, it was his fault ;) I actually moved the link to the top of the struct, since they were all on the same page, and made it a normal link to 2.4. > https://codereview.appspot.com/35790047/diff/40001/testing/mgo.go > File testing/mgo.go (right): > > https://codereview.appspot.com/35790047/diff/40001/testing/mgo.go#newcode59 > testing/mgo.go:59: const mgoDialTimeout = 15 * time.Second > I didn't see a mention of why this had to change. Is it just the load of > bringing up multiple servers? Honestly, I changed it during testing and forgot to change it back. 15 seconds is fine, we shouldn't really ever time out either way.
Sign in to reply to this message.
On 2013/12/12 12:18:12, rog wrote: > > https://codereview.appspot.com/35790047/diff/20001/testing/mgo.go > File testing/mgo.go (right): > > https://codereview.appspot.com/35790047/diff/20001/testing/mgo.go#newcode43 > testing/mgo.go:43: // Server holds the running MongoDB command. > On 2013/12/11 12:05:07, rog wrote: > > I'm not sure about exporting all these fields. > > A suggestion below. > > Oops, looks like I forgot to post the suggestion. > > Just wondering why we don't have something more like > this, with no fields exported in MgoInstance. > Then it's obvious how MgoInstance should be created. > > // MgoInstance returns a new running mongo server instance > // with the given extra command line arguments. > func NewMgoInstance(params ...string) (*MgoInstance, error) > > func (inst *MgoInstance) Reset() error > func (inst *MgoInstance) Addr() string > func (inst *MgoInstance) Port() int > > > ... other methods. Yep, good idea, did that, though I kept params as a struct value since in theory, you can change them and then restart mongo with the new params. > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > File environs/replicaset/replicaset.go (right): > > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > environs/replicaset/replicaset.go:35: // Value is optional, defaults to false. > // This value is optional; it defaults to false. > > > and similarly below? done > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > File environs/replicaset/replicaset_test.go (right): > > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > environs/replicaset/replicaset_test.go:59: func() { > Can we just move this out into its own function? > The anonymous function is cute but not a significant > win in this case IMO. Yeah, sorry, that was a holdover from when it was a much, much smaller function. Moved it out to a real function. > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > environs/replicaset/replicaset_test.go:64: if err != nil { > c.Assert(err, gc.IsNil) > > and similarly below. This was a holdover from when they were in tests and not in the suite setup.... fixed. > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > environs/replicaset/replicaset_test.go:78: if !reflect.DeepEqual(mems, > expectedMembers) { > c.Assert(mems, gc.DeepEquals, expectedMembers) done. > https://codereview.appspot.com/35790047/diff/40001/environs/replicaset/replic... > environs/replicaset/replicaset_test.go:82: // now add some data so we get a more > real-life test > perhaps this might work well in a separate function. Yep, you're right, done.
Sign in to reply to this message.
|
