-
Notifications
You must be signed in to change notification settings - Fork 454
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
[dbnode] Large tiles writer tests #2608
[dbnode] Large tiles writer tests #2608
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.
LGTM with some minor nits, just want to understand if the datapoint values are checked somewhere.
"testing" | ||
"time" | ||
|
||
"github.com/golang/mock/gomock" |
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.
Move to a separate block.
"github.com/m3db/m3/src/x/context" | ||
"github.com/m3db/m3/src/x/ident" | ||
xtime "github.com/m3db/m3/src/x/time" | ||
"github.com/stretchr/testify/require" |
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.
Move to a separate block.
readInfoFileResults := ReadInfoFiles(filePathPrefix, testNs1ID, 0, 16, nil, persist.FileSetFlushType) | ||
require.Equal(t, 1, len(readInfoFileResults)) | ||
for _, result := range readInfoFileResults { | ||
require.NoError(t, result.Err.Error()) |
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.
Since you have already established that len(readInfoFileResults) == 1
, and are working just with readInfoFileResults[0].Info
below, it might be more consistent to directly check readInfoFileResults[0].Err
here as well (instrad of the loop).
} | ||
|
||
r := newTestReader(t, filePathPrefix) | ||
readTestDataWithOrderOpt(t, r, 0, testWriterStart, expectEntries, true) |
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.
I might be missing something, but is there any place where it checks the actual datapoint values written?
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.
It compares bytes passed: https://github.com/m3db/m3/blob/gg/large-tiles-writer-tests/src/dbnode/persist/fs/read_write_test.go#L241
When we encode datapoints into bytes, we store bytes representation with series and later compare it: https://github.com/m3db/m3/pull/2608/files#diff-d62e9a6b4cbf4aaa3f88808e12f7952dR215
Moved to Writer PR here: #2618 |
Unit test for larges tiles writer.