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

storage: Disable campaign-on-wake when receiving raft messages #26463

Merged
merged 2 commits into from
Jun 6, 2018

Conversation

bdarnell
Copy link
Contributor

@bdarnell bdarnell commented Jun 6, 2018

When the incoming message is a MsgVote (which is likely the case for
the first message received by a quiesced follower), immediate
campaigning will cause the election to fail.

This is similar to reverting commit 44e3977, but only disables
campaigning in one location.

Fixes #26391

Release note: None

This prevents firewall prompts on macos (which never seem to remember
my answer) when running tests in a loop.

Release note: None
@bdarnell bdarnell requested review from tbg, a-robinson and a team June 6, 2018 15:12
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@a-robinson
Copy link
Contributor

:lgtm: but I didn't really vet every call point. Let me know if you'd like me to. Thanks for tracking this down!


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@petermattis
Copy link
Collaborator

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


pkg/cmd/roachtest/cluster.go, line 721 at r1 (raw file):

	if local {
		// This avoids annoying firewall prompts on macos
		args = append(args, "--args", "--host=127.0.0.1")

Does that work? I would have though you needed --args=--host=127.0.0.1.


pkg/storage/replica.go, line 3456 at r2 (raw file):

	// include MsgVotes), so don't campaign if we wake up our raft
	// group.
	return r.withRaftGroup(false, func(raftGroup *raft.RawNode) (bool, error) {

An inline comment such as false /* mayCampaign */ would be helpful for future readers.


Comments from Reviewable

@bdarnell
Copy link
Contributor Author

bdarnell commented Jun 6, 2018

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


pkg/cmd/roachtest/cluster.go, line 721 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Does that work? I would have though you needed --args=--host=127.0.0.1.

Yes, it works.


pkg/storage/replica.go, line 3456 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

An inline comment such as false /* mayCampaign */ would be helpful for future readers.

Maybe, but then it's likely to push the function literal onto the next line so I don't think it's a net win.


Comments from Reviewable

When the incoming message is a MsgVote (which is likely the case for
the first message received by a quiesced follower), immediate
campaigning will cause the election to fail.

This is similar to reverting commit 44e3977, but only disables
campaigning in one location.

Fixes cockroachdb#26391

Release note: None
@bdarnell
Copy link
Contributor Author

bdarnell commented Jun 6, 2018

bors r+

craig bot pushed a commit that referenced this pull request Jun 6, 2018
26441: distsql: add NewFinalIterator to the rowIterator interface r=asubiotto a=asubiotto

Some implementations of the rowIterator interface would destroy rows as
they were iterated over to free memory eagerly. NewFinalIterator is
introduced in this change to provide non-reusable behavior and
NewIterator is explicitly described as reusable.

A reusable iterator has been added to the memRowContainer to satisfy
these new interface semantics.

Release note: None

26463: storage: Disable campaign-on-wake when receiving raft messages r=bdarnell a=bdarnell

When the incoming message is a MsgVote (which is likely the case for
the first message received by a quiesced follower), immediate
campaigning will cause the election to fail.

This is similar to reverting commit 44e3977, but only disables
campaigning in one location.

Fixes #26391

Release note: None

26469: lint: Fix a file descriptor leak r=bdarnell a=bdarnell

This leak is enough to cause `make lintshort` fail when run under the
default file descriptor limit on macos (256).

Release note: None

26470: build: Pin go.uuid to the version currently in use r=bdarnell a=bdarnell

Updates #26332

Release note: None

Co-authored-by: Alfonso Subiotto Marqués <alfonso@cockroachlabs.com>
Co-authored-by: Ben Darnell <ben@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Jun 6, 2018

Build succeeded

@craig craig bot merged commit da2025f into cockroachdb:master Jun 6, 2018
@tbg
Copy link
Member

tbg commented Jun 6, 2018

LGTM 👍 going to try this on my 100k range cluster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants