Skip to content

storage: Don't add NoopRequests to the command queue#10470

Merged
bdarnell merged 2 commits intocockroachdb:masterfrom
bdarnell:command-queue-noop
Nov 6, 2016
Merged

storage: Don't add NoopRequests to the command queue#10470
bdarnell merged 2 commits intocockroachdb:masterfrom
bdarnell:command-queue-noop

Conversation

@bdarnell
Copy link
Copy Markdown
Contributor

@bdarnell bdarnell commented Nov 5, 2016

Doing so was creating unnecessary contention in the command queue for the empty key.

Discovered during investigation of #10427


This change is Reviewable

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Nov 5, 2016

:lgtm:


Reviewed 1 of 1 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@tbg
Copy link
Copy Markdown
Member

tbg commented Nov 5, 2016

:lgtm:, could you check that make test PKG=./storage COCKROACH_PROPOSER_EVALUATED_KV=true TESTS=Command still passes as well?


Reviewed 1 of 1 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Copy link
Copy Markdown
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

LGTM

count := 0
cq.tree.Do(func(i interval.Interface) bool {
c := i.(*cmd)
keys = append(keys, fmt.Sprintf("%s-%s", roachpb.Key(c.key.Start), roachpb.Key(c.key.End)))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

keys.PrettyPrintRange?

This has nicer error reporting without including test numbers in every
message.
Doing so was creating unnecessary contention in the command queue for
the empty key.
@bdarnell
Copy link
Copy Markdown
Contributor Author

bdarnell commented Nov 6, 2016

Tests still pass with proposer-evaluated-kv.

keys.PrettyPrintRange would require a larger refactoring of commandQueue.String than I feel like undertaking at this point.

@bdarnell bdarnell merged commit fc842bc into cockroachdb:master Nov 6, 2016
@bdarnell bdarnell deleted the command-queue-noop branch November 6, 2016 05:24
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.

4 participants