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

Add a tsdb writer interface #7950

Merged
merged 9 commits into from Oct 12, 2020

Conversation

JessicaGreben
Copy link
Contributor

@JessicaGreben JessicaGreben commented Sep 19, 2020

This PR adds one file from #7586 that is copy/pasted into another PR #7675 .

The change in the PR adds a tsdb writer interface and also a tsdbWriter object that implements the interface. This is nice to have so we have a simple wrapper around writing time series to tsdb blocks.

Signed-off-by: jessicagreben jessicagrebens@gmail.com

Signed-off-by: jessicagreben <jessicagrebens@gmail.com>
@brian-brazil
Copy link
Contributor

We shouldn't be adding dead code, PRs should generally make sense standalone. Maybe there's some tests that could use it?

@roidelapluie
Copy link
Member

Is that the same as teststorage?

@roidelapluie
Copy link
Member

what is the reason to extract it from above PR's?

@JessicaGreben
Copy link
Contributor Author

JessicaGreben commented Sep 20, 2020

@brian-brazil while looking for a some tests that could use this code, I came across this code:
https://github.com/prometheus/prometheus/blob/master/tsdb/tsdbblockutil.go

It doesn't seem like we need both of these implementations since they are mostly the same. I will close this PR and use the tsdbblockutil code in #7675 instead.

@roidelapluie I wanted to extract it from the above PRs just to get it merged in easier.

@codesome
Copy link
Member

@JessicaGreben Note that functions in tsdbblockutil require all samples to be presented beforehand. It would be nice (or even a requirement) to make them iterators for streaming samples for the backfill.

@bwplotka
Copy link
Member

Happy to reuse more, but we might want to add there some writer like we did in our PRs, for streaming / appender functionality indeed.

@JessicaGreben
Copy link
Contributor Author

@codesome @bwplotka agree about the iterator would be nice.

Should I reopen this PR and try to combine the two? Or is there a use for both?

@codesome
Copy link
Member

If the writer interface is more handy for backfilling purposes (which I would believe so), I would say let's re-open and have the writer interface as is and use it inside CreateBlock. That way it won't be a dead code.

@JessicaGreben JessicaGreben reopened this Sep 21, 2020
@codesome
Copy link
Member

(Which means we need to remove CreateHead, that is not used anywhere else I think other than CreateBlock)

Signed-off-by: jessicagreben <jessicagrebens@gmail.com>
Signed-off-by: jessicagreben <jessicagrebens@gmail.com>
@JessicaGreben
Copy link
Contributor Author

JessicaGreben commented Sep 21, 2020

Ok so I just added the change to use the block writer in the tsdbblockutil code, but still nothing is using the tsdbblockutil code. So I'm going to see if I can fix that by adding it to a test. Also I will add some other tests for this writer code.

@codesome
Copy link
Member

I think block_test.go uses it

@JessicaGreben
Copy link
Contributor Author

JessicaGreben commented Sep 21, 2020

@codesome I don't see the code from tsdbblockutil.go used in block_test.go, but looks like it might be easy to add, let me add that

tsdb/blockwriter.go Outdated Show resolved Hide resolved
tsdb/tsdbblockutil.go Outdated Show resolved Hide resolved
tsdb/tsdbblockutil.go Outdated Show resolved Hide resolved
tsdb/tsdbblockutil.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
@codesome
Copy link
Member

Just came across this

// createBlock creates a block with given set of series and returns its dir.
func createBlock(tb testing.TB, dir string, series []storage.Series) string {
chunkDir, err := ioutil.TempDir("", "chunk_dir")
testutil.Ok(tb, err)
defer func() { testutil.Ok(tb, os.RemoveAll(chunkDir)) }()
head := createHead(tb, nil, series, chunkDir)
defer func() { testutil.Ok(tb, head.Close()) }()
return createBlockFromHead(tb, dir, head)
}
func createBlockFromHead(tb testing.TB, dir string, head *Head) string {
compactor, err := NewLeveledCompactor(context.Background(), nil, log.NewNopLogger(), []int64{1000000}, nil)
testutil.Ok(tb, err)
testutil.Ok(tb, os.MkdirAll(dir, 0777))
// Add +1 millisecond to block maxt because block intervals are half-open: [b.MinTime, b.MaxTime).
// Because of this block intervals are always +1 than the total samples it includes.
ulid, err := compactor.Write(dir, head, head.MinTime(), head.MaxTime()+1, nil)
testutil.Ok(tb, err)
return filepath.Join(dir, ulid.String())
}
func createHead(tb testing.TB, w *wal.WAL, series []storage.Series, chunkDir string) *Head {
head, err := NewHead(nil, nil, w, DefaultBlockDuration, chunkDir, nil, DefaultStripeSize, nil)
testutil.Ok(tb, err)
app := head.Appender(context.Background())
for _, s := range series {
ref := uint64(0)
it := s.Iterator()
for it.Next() {
t, v := it.At()
if ref != 0 {
err := app.AddFast(ref, t, v)
if err == nil {
continue
}
}
ref, err = app.Add(s.Labels(), t, v)
testutil.Ok(tb, err)
}
testutil.Ok(tb, it.Err())
}
testutil.Ok(tb, app.Commit())
return head
}

We can get rid of this and bring back CreateHead in the blockutil and replace those above with the methods in the blockutil. But a small test just for the writer would also make sense.

Signed-off-by: jessicagreben <jessicagrebens@gmail.com>
jessicagreben added 3 commits September 27, 2020 07:45
Signed-off-by: jessicagreben <jessicagrebens@gmail.com>
Signed-off-by: jessicagreben <jessicagrebens@gmail.com>
Signed-off-by: jessicagreben <jessicagrebens@gmail.com>
@JessicaGreben
Copy link
Contributor Author

@codesome Thanks for your help and comments 😄

I think I addressed all your comments. When/if you have any free time, it would be great to get another review from you.

About your last comment (re:#7950 (comment)), I ended up not deleting the function since it resulted in a lot of changes. Instead I call the tsdbblockutil CreateBlock method from inside the block_test.go createBlock method to keep the function signature the same and that made the change much easier.

Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

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

Almost there! Some nits and we need to update the test.

Additionally, we are still requiring initHead to be called from outside. It should be part of NewBlockWriter.

tsdb/tsdbblockutil.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
tsdb/blockwriter_test.go Outdated Show resolved Hide resolved
tsdb/blockwriter_test.go Outdated Show resolved Hide resolved
Signed-off-by: jessicagreben <jessicagrebens@gmail.com>
@bwplotka bwplotka changed the title add a tsdb writer interface Add a tsdb writer interface Oct 5, 2020
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

This is perfect from my side. Thanks for this @JessicaGreben ❤️

LGTM!

tsdb/blockwriter.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

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

LGTM! Couple of nits, in test and the about duration which Brian mentioned

tsdb/blockwriter_test.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
Signed-off-by: jessicagreben <jessicagrebens@gmail.com>
@JessicaGreben
Copy link
Contributor Author

@codesome thanks for your help. I addressed the final comments. Let me know what you think...hopefully we can merge :)

@bwplotka bwplotka merged commit 90680b0 into prometheus:master Oct 12, 2020
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants