*: Address issues with tracking disk usage as capacity minus availability #17733

a-robinson · 2017-08-17T17:42:18Z

See commit descriptions for details. The first commit is important to improving stats-based rebalancing. The second commit isn't necessary, but I got halfway done with it while thinking through the first one so included it anyways. I'm happy to separate it out for later if desired.

Addresses #17691 and #13548. I'll regenerate embedded.go before submitting.

cockroach-teamcity · 2017-08-17T17:42:24Z

This change is

a-robinson · 2017-08-17T17:42:51Z

pkg/storage/store.go

 	var totalWritesPerSecond float64
 	bytesPerReplica := make([]float64, 0, capacity.RangeCount)
 	writesPerReplica := make([]float64, 0, capacity.RangeCount)
 	newStoreReplicaVisitor(s).Visit(func(r *Replica) bool {
 		if r.ownsValidLease(now) {
 			leaseCount++
 		}
-		bytesPerReplica = append(bytesPerReplica, float64(r.GetMVCCStats().LiveBytes))
+		mvccStats := r.GetMVCCStats()
+		// TODO(DONOTSUBMIT): Which MVCCStats fields should we use for this?


cc @petermattis for his opinion on what's most accurate here.

BramGruneir · 2017-08-17T17:50:26Z

With one nit and obviously waiting on Peter's response.

Reviewed 9 of 10 files at r1.
Review status: 4 of 21 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

pkg/roachpb/metadata.proto, line 115 at r1 (raw file):

  // to another actually removes its bytes from the source store even though
  // RocksDB may not actually reclaim the physical disk space for a while.
  optional int64 logical_bytes = 9 [(gogoproto.nullable) = false];

Is it common practice to not have the index in order in the proto file??

Comments from Reviewable

BramGruneir · 2017-08-17T17:51:06Z

Oh, there's a new commit. I only looked at the first one.

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

Comments from Reviewable

BramGruneir · 2017-08-17T17:54:38Z

Still LGTM.

Reviewed 16 of 17 files at r2.
Review status: 19 of 21 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

a-robinson · 2017-08-17T17:57:00Z

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

pkg/roachpb/metadata.proto, line 115 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Is it common practice to not have the index in order in the proto file??

It wasn't uncommon at Google, and while I only checked a couple of our proto files we've done it before for roachpb.Transaction and storage.Header.

Comments from Reviewable

petermattis · 2017-08-17T18:02:34Z

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

pkg/roachpb/metadata.proto, line 115 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

It wasn't uncommon at Google, and while I only checked a couple of our proto files we've done it before for roachpb.Transaction and storage.Header.

The proto fields should be grouped in the natural order, ignoring tag numbers. Forcing the fields into tag number order reduces readability. For the most part, the tag numbers aren't something you care about.

pkg/storage/store.go, line 2280 at r2 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

cc @petermattis for his opinion on what's most accurate here.

Depends on the purpose? LiveBytes+SysBytes captures the long term size of data in the range after GC has been performed. KeyBytes+ValBytes+SysBytes captures the instantaneous size of data in the range (ie. it includes historical data that will eventually be GC'd). Note that IntentBytes is included in LiveBytes.

My guess is that KeyBytes+ValBytes+SysBytes is the better choice.

Comments from Reviewable

tbg · 2017-08-17T18:04:01Z

and nice, this was an ugly wart!

Reviewed 10 of 10 files at r1, 16 of 17 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

pkg/roachpb/metadata.go, line 275 at r3 (raw file):

// FractionUsed computes the fraction of storage capacity that is in use.
func (sc StoreCapacity) FractionUsed() float64 {

Where are we still using FractionUsed()? For anything that matters?

pkg/roachpb/metadata.proto, line 115 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The proto fields should be grouped in the natural order, ignoring tag numbers. Forcing the fields into tag number order reduces readability. For the most part, the tag numbers aren't something you care about.

I'm not aware of a fixed precedent, and while I like to keep them in order, I definitely see how it makes sense the way it is in this diff.

pkg/storage/allocator_scorer.go, line 523 at r1 (raw file):

		if log.V(2) {
			log.Infof(ctx,
				"s%d: should-rebalance(bad-fit): - balanceScore=%s, capacity=(%v), rangeInfo=%+v, "+

What's that lonely - for? Do you mean -(a + b + c)? I also don't understand bad-fit. Similarly the other messages.

pkg/storage/allocator_scorer.go, line 567 at r1 (raw file):

	if log.V(3) {
		log.Infof(ctx,
			"s%d: should-not-rebalance: - balanceScore=%s, capacity=(%v), rangeInfo=%+v, "+

What does capacity print like with %v? It should print like meanDiskUsage, right?

pkg/storage/store.go, line 2280 at r2 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

cc @petermattis for his opinion on what's most accurate here.

I think KeyBytes+ValBytes+SysBytes is the right one, the others ignore historical versions and I don't see a reason for that.
One could argue about SysBytes being included, but it should typically be small, and if it isn't better to have rebalances happening that the user doesn't understand than to go out of balance as a result.

Comments from Reviewable

petermattis · 2017-08-17T18:24:39Z

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

pkg/storage/store.go, line 2280 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

I think KeyBytes+ValBytes+SysBytes is the right one, the others ignore historical versions and I don't see a reason for that.
One could argue about SysBytes being included, but it should typically be small, and if it isn't better to have rebalances happening that the user doesn't understand than to go out of balance as a result.

I would definitely include SysBytes. It should be small, but sometimes it isn't.

Comments from Reviewable

a-robinson · 2017-08-17T18:59:47Z

TFTRs!

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

pkg/roachpb/metadata.go, line 275 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Where are we still using FractionUsed()? For anything that matters?

The max-capacity check that attempts to avoid filling a node past 95% - https://github.com/cockroachdb/cockroach/blob/master/pkg/storage/allocator_scorer.go#L966

pkg/storage/allocator_scorer.go, line 523 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

What's that lonely - for? Do you mean -(a + b + c)? I also don't understand bad-fit. Similarly the other messages.

It's not meant to be there, must have either been a mistake or a leftover from some prior version of the format string.

disk-full/bad-fit/etc. are indicating the reason why we should-rebalance.

pkg/storage/allocator_scorer.go, line 567 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

What does capacity print like with %v? It should print like meanDiskUsage, right?

It prints as defined by (StoreCapacity).String() in roachpb.metadata.go. I've conventionally used %v for structs for a String method, but can switch to %s if that's our standard.

pkg/storage/store.go, line 2280 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I would definitely include SysBytes. It should be small, but sometimes it isn't.

Hm, I was going to include SysBytes, but it'd nice be to reuse an existing method and it seems a bit odd that MVCCStats.Total() doesn't include SysBytes. It may just be awkward naming, but adding an additional total-bytes method that returns more than Total would definitely be unintuitive.

It seems to me like either SysBytes should be included in Total (and thus used everywhere) or not used here either. I've switched to just using Total, but let me know if I should do otherwise.

Comments from Reviewable

tbg · 2017-08-17T19:18:05Z

Reviewed 20 of 20 files at r4, 17 of 17 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

pkg/storage/allocator_scorer.go, line 523 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

It's not meant to be there, must have either been a mistake or a leftover from some prior version of the format string.

disk-full/bad-fit/etc. are indicating the reason why we should-rebalance.

And the + later in the line?

pkg/storage/allocator_scorer.go, line 567 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

It prints as defined by (StoreCapacity).String() in roachpb.metadata.go. I've conventionally used %v for structs for a String method, but can switch to %s if that's our standard.

Sorry, I'm fine with %v, was just curious whether that boils down to humanizeutil.IBytes(...) as well. (not important).

pkg/storage/store.go, line 2280 at r2 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Hm, I was going to include SysBytes, but it'd be to reuse an existing method and it seems a bit odd that [MVCCStats.Total()'](https://github.com/cockroachdb/cockroach/blob/master/pkg/storage/engine/enginepb/mvcc.go#L24) doesn't include SysBytes. It may just be awkward naming, but adding an additional total-bytes method that returns more than Total` would definitely be unintuitive.

It seems to me like either SysBytes should be included in Total (and thus used everywhere) or not used here either. I've switched to just using Total, but let me know if I should do otherwise.

Hmm, I'm not totally sure, but I'm afraid we're doing something weird in SysBytes. I think we use it for all keys that have the localPrefix, but that in turn contains both data that is local and replicated (various store-local things, but also, for example, the RangeDescriptor and everything else in LocalRangePrefix = roachpb.Key(makeKey(localPrefix, roachpb.RKey("k")))). Some comments in that area suggest that we're intentionally mixing unreplicated and replicated keys for, I guess, performance, so perhaps we're doing everything right -- not obvious to me, though. I suppose the consistency checker would find violations here, so perhaps it's all right.

Either way, ISTM that MVCCStats should really only contain the replicated parts and so we should include SysBytes in total, though there's no pressing need to do so right now. I'll file something to investigate: #17737

Comments from Reviewable

Fixes cockroachdb#17691

Fixes cockroachdb#13548

a-robinson · 2017-08-17T19:41:58Z

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

pkg/storage/allocator_scorer.go, line 567 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Sorry, I'm fine with %v, was just curious whether that boils down to humanizeutil.IBytes(...) as well. (not important).

No, it boils down to (StoreCapacity).String(), which includes much more.

pkg/storage/store.go, line 2280 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Hmm, I'm not totally sure, but I'm afraid we're doing something weird in SysBytes. I think we use it for all keys that have the localPrefix, but that in turn contains both data that is local and replicated (various store-local things, but also, for example, the RangeDescriptor and everything else in LocalRangePrefix = roachpb.Key(makeKey(localPrefix, roachpb.RKey("k")))). Some comments in that area suggest that we're intentionally mixing unreplicated and replicated keys for, I guess, performance, so perhaps we're doing everything right -- not obvious to me, though. I suppose the consistency checker would find violations here, so perhaps it's all right.

Either way, ISTM that MVCCStats should really only contain the replicated parts and so we should include SysBytes in total, though there's no pressing need to do so right now. I'll file something to investigate: #17737

Thanks!

Comments from Reviewable

a-robinson · 2017-08-18T14:41:51Z

So I think this is good to go, but now we don't have a graph in the admin UI that directly corresponds to what we're using to rebalance. We export metrics for KeyBytes and ValBytes, but not for TotalBytes, which is what would be ideal. I don't imagine we have the ability to add two metrics together into a single line in our UI, do we, @mrtracy? Adding a new metric would be redundant (and confusing to anyone trying to interpret the difference between all these fields), but is worth it if we can't graph it otherwise.

petermattis · 2017-08-18T14:51:59Z

Feels like the "Live Bytes per Store" graph should be renamed to "Logical Bytes per Store". Seems useful to graph the metric the rebalancer is using.

a-robinson · 2017-08-18T14:53:04Z

Right, but it's not a matter of renaming, it's a matter of having the right data to graph.

petermattis · 2017-08-18T17:16:52Z

Ok. Let's get this change merged and file a follow-on issue to change the graph.

Fixes cockroachdb#17912. This was broken by cockroachdb#17733.

a-robinson requested review from a team August 17, 2017 17:42

a-robinson commented Aug 17, 2017

View reviewed changes

a-robinson force-pushed the capacity branch from 121a73f to e8d8f81 Compare August 17, 2017 17:54

a-robinson force-pushed the capacity branch from e8d8f81 to 87c6e8d Compare August 17, 2017 18:59

tbg mentioned this pull request Aug 17, 2017

engine: MVCCStats.Total() should include SysBytes (?) #17737

Closed

a-robinson added 2 commits August 17, 2017 15:34

storage: Rebalance based on logical bytes, not fraction of disk used

3baedfe

Fixes cockroachdb#17691

*: Add Used field to StoreCapacity to reduce disk accounting confusion

050bc89

Fixes cockroachdb#13548

a-robinson force-pushed the capacity branch from 87c6e8d to 050bc89 Compare August 17, 2017 19:42

a-robinson mentioned this pull request Aug 17, 2017

ui: Intermediate page with error message always loads first when going to range debug page #17744

Closed

a-robinson merged commit 522f43a into cockroachdb:master Aug 18, 2017

This was referenced Aug 18, 2017

ui: Change LiveBytes graph on Replicas page to reflect metric used in rebalancing #17753

Closed

acceptance: stats-based rebalancing makes Allocator Test 3 to 5 10G flaky #17685

Closed

bdarnell mentioned this pull request Aug 24, 2017

"Capacity Used" showing as 0.00% despite using 7% capacity. #17912

Closed

a-robinson mentioned this pull request Aug 24, 2017

ui: fix "capacity used" percentage in summary bar #17916

Merged

a-robinson added a commit to a-robinson/cockroach that referenced this pull request Aug 24, 2017

ui: fix "capacity used" percentage in summary bar

1495641

Fixes cockroachdb#17912. This was broken by cockroachdb#17733.

couchand mentioned this pull request Sep 21, 2017

perf: analyze INSERT INTO .... VALUES(...) ON CONFLICT DO UPDATE SET n = n+1 #18657

Closed

a-robinson deleted the capacity branch May 18, 2018 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Address issues with tracking disk usage as capacity minus availability #17733

*: Address issues with tracking disk usage as capacity minus availability #17733

a-robinson commented Aug 17, 2017

cockroach-teamcity commented Aug 17, 2017

a-robinson Aug 17, 2017

BramGruneir commented Aug 17, 2017

BramGruneir commented Aug 17, 2017

BramGruneir commented Aug 17, 2017

a-robinson commented Aug 17, 2017

petermattis commented Aug 17, 2017

tbg commented Aug 17, 2017

petermattis commented Aug 17, 2017

a-robinson commented Aug 17, 2017 •

edited

Loading

tbg commented Aug 17, 2017

a-robinson commented Aug 17, 2017

a-robinson commented Aug 18, 2017

petermattis commented Aug 18, 2017

a-robinson commented Aug 18, 2017

petermattis commented Aug 18, 2017

*: Address issues with tracking disk usage as capacity minus availability #17733

*: Address issues with tracking disk usage as capacity minus availability #17733

Conversation

a-robinson commented Aug 17, 2017

cockroach-teamcity commented Aug 17, 2017

a-robinson Aug 17, 2017

Choose a reason for hiding this comment

BramGruneir commented Aug 17, 2017

BramGruneir commented Aug 17, 2017

BramGruneir commented Aug 17, 2017

a-robinson commented Aug 17, 2017

petermattis commented Aug 17, 2017

tbg commented Aug 17, 2017

petermattis commented Aug 17, 2017

a-robinson commented Aug 17, 2017 • edited Loading

tbg commented Aug 17, 2017

a-robinson commented Aug 17, 2017

a-robinson commented Aug 18, 2017

petermattis commented Aug 18, 2017

a-robinson commented Aug 18, 2017

petermattis commented Aug 18, 2017

a-robinson commented Aug 17, 2017 •

edited

Loading