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: allocate a more conservative slice of raft log entries #12100

Merged
merged 1 commit into from Dec 6, 2016
Merged

storage: allocate a more conservative slice of raft log entries #12100

merged 1 commit into from Dec 6, 2016

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Dec 6, 2016

Note that this currently doesn't reallocate if the raft log entry cache is empty.

Also, we should probably pass a pre-allocated slice into the raft log entry cache.


This change is Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


pkg/storage/replica_raftstorage.go, line 100 at r1 (raw file):

	// Reallocate `ents` if we expect to have many more log entries.
	if numEnts := uint64(len(ents)); numEnts > 0 {

I think there is a common case here where numEnts == 0.


pkg/storage/replica_raftstorage.go, line 101 at r1 (raw file):

	// Reallocate `ents` if we expect to have many more log entries.
	if numEnts := uint64(len(ents)); numEnts > 0 {
		expectedEnts := (hi - lo) * size / numEnts

This is computing expectedSize, right? To get expectedEnts I think you need to multiply by size / maxBytes. Or maybe I'm not thinking clearly here.

This feels overly complicated. I was thinking we'd do something like:

  n := hi-lo
  if n > 100 {
    n = 100
  }
  ents = append(make([]raftpb.Entry, 0, n), ents...)

Comments from Reviewable

Allow allocating before calling (*raftEntryCache).getEntries.
@tamird
Copy link
Contributor Author

tamird commented Dec 6, 2016

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.


pkg/storage/replica_raftstorage.go, line 100 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I think there is a common case here where numEnts == 0.

Done.


pkg/storage/replica_raftstorage.go, line 101 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This is computing expectedSize, right? To get expectedEnts I think you need to multiply by size / maxBytes. Or maybe I'm not thinking clearly here.

This feels overly complicated. I was thinking we'd do something like:

  n := hi-lo
  if n > 100 {
    n = 100
  }
  ents = append(make([]raftpb.Entry, 0, n), ents...)

Alright, done.

Also moved allocation outside of getEntries.


Comments from Reviewable

@petermattis
Copy link
Collaborator

:lgtm:


Review status: 0 of 4 files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@tamird tamird merged commit 0d2ec04 into cockroachdb:master Dec 6, 2016
@tamird tamird deleted the allocate-less-raft-entry branch December 6, 2016 18:48
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

2 participants