storage: allow multiple concurrent snapshots

[tamird]

@cuongdo this is on top of #7299 - I bet the degradation we saw with this change originally was the result of that issue. Can you test it?

---

This change is

[petermattis]

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

storage/store.go, line 673 [r2] (raw file):

  raftElectionTimeout := time.Duration(sc.RaftElectionTimeoutTicks) * sc.RaftTickInterval
  sc.concurrentSnapshotLimit = 5

Per my earlier comment, let's default this to 1 and perhaps allow an env var to override. I'd like to see concrete numbers showing there is a benefit to raising the number of concurrent snapshots. I worry about allowing multiple concurrent snapshots while we have the existing memory issues with snapshots.

---

Comments from Reviewable

[tamird]

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

storage/store.go, line 673 [r2] (raw file):

Previously, petermattis (Peter Mattis) wrote…

Per my earlier comment, let's default this to 1 and perhaps allow an env var to override. I'd like to see concrete numbers showing there is a benefit to raising the number of concurrent snapshots. I worry about allowing multiple concurrent snapshots while we have the existing memory issues with snapshots.

Will do.

---

Comments from Reviewable

[bdarnell]

I'd rather hold off on this change until we demonstrate stability with the current version, especially since I agree with Peter that it is unlikely that allowing more snapshots will make sense until we can move to streaming snapshots.

[cuongdo]

Agreed. Let's let the dust settle from the recent Raft transport and
reservations changes first.

On Fri, Jun 17, 2016 at 12:22 PM Ben Darnell [email protected]
wrote:

I'd rather hold off on this change until we demonstrate stability with the
current version, especially since I agree with Peter that it is unlikely
that allowing more snapshots will make sense until we can move to streaming
snapshots.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7307 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABffphosEw8loyw4Eh_17eFbuaOm3uwOks5qMsnXgaJpZM4I4Tmk
.

[tamird]

Sure. I've rebased this, so it should be good to go whenever we're ready to start experimenting with it again.

---

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

---

Comments from Reviewable

[tamird]

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

---

storage/store.go, line 673 [r2] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Will do.

Done.

---

Comments from Reviewable

[tamird]

Rebased on top of streaming snapshots, much simpler now.

[jordanlewis]

Is there any way that we can test this change with more than 1 concurrent snapshot before merging? I see that the default is 1, but I'm nervous that we'll see problems with snapshots when we adjust that configuration. I think it would be nice to try to catch potential problems with concurrent snapshots with a test before we merge this.

The code , though.

---

Reviewed 7 of 7 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

Comments from Reviewable

[petermattis]

I like getting rid of the use of atomic here. It would be nice to have evidence that we'll ever set this value larger than 1.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

Comments from Reviewable

[tamird]

@jordanlewis updated the default to 2 -- I guess it doesn't make much sense to merge this before the default is changed. The tests still pass (though maybe you had some specific tests in mind).

@petermattis indeed -- hard to say if or when we will. Perhaps the chaos monkeys will tell. On the other hand, it may be good to expose concurrency bugs while the streaming snapshots change is fresh in everyone's minds.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[petermattis]

Can't we show via some local workload that a value larger than 1 is worthwhile?

---

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[bdarnell]

The code change LGTM, but I'd rather keep the default at 1 until we can show that this is both safe and beneficial.

[tamird]

OK, updated the default to 1.

[petermattis]

---

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/storage/store.go, line 778 at r4 (raw file):

	}
	if sc.concurrentSnapshotLimit == 0 {
		sc.concurrentSnapshotLimit = envutil.EnvOrDefaultInt("COCKROACH_CONCURRENT_SNAPSHOT_LIMIT", 1)

Can you mind adding a small comment here: NB: be careful about setting this value higher than 1 as it will degrade client throughput.

---

Comments from Reviewable

[tamird]

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

---

pkg/storage/store.go, line 778 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Can you mind adding a small comment here: NB: be careful about setting this value higher than 1 as it will degrade client throughput.

Done.

---

Comments from Reviewable