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

fix(tsm1): fix wal's totalOldDiskSize statistics #20811

Merged

Conversation

StoneYunZhao
Copy link
Contributor

@StoneYunZhao StoneYunZhao commented Feb 28, 2021

The value of the totalOldDiskSize in WAL is wrong. There are two reason caused the inaccuracy.

Reason 1 caused great inaccuracies: TotalOldDiskSize should add rather than set the current segment bytes when rolling a new segment.

Reason 2 caused minor inaccuracies: Must skip the current segment size when statistic the totalOldDiskSize, because total disk size is computed by following method:

func (l *WAL) DiskSizeBytes() int64 {
	return atomic.LoadInt64(&l.stats.OldBytes) + atomic.LoadInt64(&l.stats.CurrentBytes)
}

@danxmoran danxmoran self-requested a review March 1, 2021 15:20
@danxmoran danxmoran self-assigned this Mar 1, 2021
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with this code, so feel free to point out if testing any of the changes would be incredibly hard

tsdb/engine/tsm1/wal.go Show resolved Hide resolved
tsdb/engine/tsm1/wal.go Show resolved Hide resolved
tsdb/engine/tsm1/wal.go Show resolved Hide resolved
@StoneYunZhao
Copy link
Contributor Author

ree to point out if

Tests added.

I compared monitor data from measurement influxdb_tsm1_wal scraped by telegraf and disk usage of shard directory under wal directory from Linux OS, and confirmed that this error exists.

Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test makes sense to me, and I'm 👍 to introducing testify. Some style comments about cleaning up the test

@@ -701,6 +703,124 @@ func TestWALRollSegment(t *testing.T) {
}
}

func TestWAL_DistSize(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func TestWAL_DistSize(t *testing.T) {
func TestWAL_DiskSize(t *testing.T) {

"testing"

"github.com/golang/snappy"
"github.com/influxdata/influxdb/v2/pkg/slices"
"github.com/influxdata/influxdb/v2/tsdb/engine/tsm1"
"github.com/stretchr/testify/assert"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

IMO makes the testing simpler, since it bails at the first mismatched expectation

test := func(w *tsm1.WAL, oldZero, curZero bool) () {
// get disk size by reading file
files, err := ioutil.ReadDir(w.Path())
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if could be replaced by require.NoError

}
if !curZero && cur == 0 {
t.Fatalf("expect current disk size larger than 0, got 0")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ifs could all be replaced by require functions

// test method DiskSizeBytes
if got, exp := w.DiskSizeBytes(), old+cur; got != exp {
t.Fatalf("expect total disk size: %d, got: %d", exp, got)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here (require.Equal I think)


if err := w.Remove(closedSegments); err != nil {
t.Fatalf("close segment error: %v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require.NoError

})

var old, cur int64
for i, f := range files {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop confuses me. Is the end goal to have:

cur = // size of last file
old = // sum of sizes of all but last file

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, would we assign cur directly outside of the loop, and have the range only loop over the files we want to sum?

}

// test Statistics
ss := w.Statistics(nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the nil here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil means no need to add additional tags


test(w, true, false)

// write with rollSegment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super familiar with this code, so I'm confused by this comment when I don't see rollSegment below. Can you clarify? Something like:

Suggested change
// write with rollSegment
// write enough points to force the segment to roll


test(w, true, true)

// write without rollSegment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// write without rollSegment
// write some points (not enough to force the segment to roll)

@StoneYunZhao
Copy link
Contributor Author

The test makes sense to me, and I'm 👍 to introducing testify. Some style comments about cleaning up the test

Thank you very much for your suggestions, the code is indeed a lot concise.

@danxmoran danxmoran merged commit 265c1f3 into influxdata:master Mar 3, 2021
@StoneYunZhao StoneYunZhao deleted the zhaoyun/wal-oldSegmentsDiskBytes branch March 4, 2021 02:16
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