-
Notifications
You must be signed in to change notification settings - Fork 493
metrics: fix w-amp calculation w/ blob files #4764
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions
metrics.go
line 189 at r1 (raw file):
// TableBytesFlushed + TableBytesCompacted + BlobBytesFlushed + BlobBytesWritten // ----------------------------------------------------------------------------- // TableBytesIn + BlobBytesFlushed
For L0, TableBytesIn is defined as "the bytes written to the WAL.". This would include BlobBytesFlushed, no?
I checked the code and it's updated here:
Line 1570 in 88e7636
l0Metrics.TableBytesIn += d.mu.mem.queue[i].logSize |
I think it's fine to keep the definition as is, but we need to divide only by TableBytesIn
here, and we need to update this line to also add BlobBytesFlushed
to it:
Line 1567 in 88e7636
l0Metrics.TableBytesIn = l0Metrics.TableBytesFlushed |
metrics.go
line 194 at r1 (raw file):
return 0 } return float64(m.TableBytesFlushed+m.TableBytesCompacted+m.BlobBytesFlushed+m.BlobBytesWritten) /
[nit] We should rename BlobBytesWritten
to BlobBytesCompacted
, it is confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @RaduBerinde)
metrics.go
line 189 at r1 (raw file):
Previously, RaduBerinde wrote…
For L0, TableBytesIn is defined as "the bytes written to the WAL.". This would include BlobBytesFlushed, no?
I checked the code and it's updated here:
Line 1570 in 88e7636
l0Metrics.TableBytesIn += d.mu.mem.queue[i].logSize I think it's fine to keep the definition as is, but we need to divide only by
TableBytesIn
here, and we need to update this line to also addBlobBytesFlushed
to it:Line 1567 in 88e7636
l0Metrics.TableBytesIn = l0Metrics.TableBytesFlushed
Good catch, done
metrics.go
line 194 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] We should rename
BlobBytesWritten
toBlobBytesCompacted
, it is confusing
Done.
Fix the w-amp calculation to properly account for blob files. We should be including all bytes written to disk in the numerator, and only bytes "in" (flushed in the case of blob files) in the denominator. Informs cockroachdb#4581.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, all discussions resolved
TFTR! |
Fix the w-amp calculation to properly account for blob files. We should be including all bytes written to disk in the numerator, and only bytes "in" (flushed in the case of blob files) in the denominator.
Informs #4581.