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: avoid unmarshaling snapshot data in Store.canApplySnapshot #7850

Closed
petermattis opened this issue Jul 15, 2016 · 4 comments
Closed
Labels
S-1-stability Severe stability issues that can be fixed by upgrading, but usually don’t resolve by restarting
Milestone

Comments

@petermattis
Copy link
Collaborator

Store.canApplySnapshot unmarshals the snapshot data (roachpb.RaftSnapshotData) in order to access the roachpb.RaftSnapshotData.RangeDescriptor which is needed for the range start and end keys. The RangeDescriptor is small in comparison to roachpb.RaftSnapshotData. We could either replicate it in RaftMessageRequest or include the start and end keys (i.e. a roachpb.Span) in RaftMessageRequest whenever a snapshot is present.

@petermattis petermattis added this to the Q3 milestone Jul 15, 2016
@bdarnell
Copy link
Contributor

We could also unmarshal into a partial message type:

message PartialRaftSnapshotData {
  optional RangeDescriptor range_descriptor = 1 [(gogoproto.nullable) = false];
}

By using the same tag numbers (and disabling unrecognized-field support), we can parse just the parts we need and skip over the rest (although just skipping over all the KV pairs takes time, so we'd be saving memory more than CPU).

@tbg
Copy link
Member

tbg commented Jul 19, 2016

cc #7830 since a solution proposed there (range reservations) would play along nicely as a container for the unmarshaled snapshot.

@petermattis petermattis added the S-1-stability Severe stability issues that can be fixed by upgrading, but usually don’t resolve by restarting label Jul 21, 2016
rjnn pushed a commit to rjnn/cockroach that referenced this issue Aug 11, 2016
In Store.canApplySnapshot, the entire snapshot is unmarshaled just to
access the range descriptor (and then unmarshaled again
later). Introduce a new proto, PartialRaftSnapshot, which only
unmarshals the range descriptor. This is not a complete fix, but saves
some memory. Close cockroachdb#7850, for now.
@rjnn
Copy link
Contributor

rjnn commented Aug 16, 2016

Partially addressed in #8365, via the strategy described in @bdarnell's comment above, but leaving this issue open as we should aim to only unmarshall the snapshot once.

@tamird tamird closed this as completed in a97e2e9 Aug 17, 2016
@tamird tamird reopened this Aug 17, 2016
@rjnn rjnn removed their assignment Aug 18, 2016
@bdarnell
Copy link
Contributor

This has been fixed with streaming snapshots; the range descriptor is now transmitted separately from the bulk of the snapshot data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-1-stability Severe stability issues that can be fixed by upgrading, but usually don’t resolve by restarting
Projects
None yet
Development

No branches or pull requests

5 participants