Skip to content

[wip] roachtest: use check-store in roachtest#36758

Closed
tbg wants to merge 1 commit intocockroachdb:masterfrom
tbg:roachtest/run-check-store
Closed

[wip] roachtest: use check-store in roachtest#36758
tbg wants to merge 1 commit intocockroachdb:masterfrom
tbg:roachtest/run-check-store

Conversation

@tbg
Copy link
Copy Markdown
Member

@tbg tbg commented Apr 11, 2019

WIP because it still finds stuff that I think is benign, and because
I need to figure out a better UX for how the result is printed/stored
in roachtest.


This is motivated by a couple of things:

  1. makes sure the command is actually useful and works
  2. gives us confidence that nothing fishy is going on
  3. gives us a convenient place to look for outliers (stats, log size).

I think it's fine to run this command against a running node (at least
this works like a charm locally), which is required for easy roachtest
integration. Perhaps @ajkr can confirm that this is fine.

PS I'm seeing the annoying message

190411 12:48:01.858427 1 storage/engine/rocksdb.go:127 [rocksdb] [/Users/tschottdorf/go/src/github.com/cockroachdb/cockroach/c-deps/rocksdb/db/version_set.cc:2566] More existing levels in DB than needed. max_bytes_for_level_multiplier may not be guaranteed.

again in this context. We need to silence that or customer support
will get asked about it all the time.

Release note: None

WIP because it still finds stuff that I think is benign, and because
I need to figure out a better UX for how the result is printed/stored
in roachtest.

----

This is motivated by a couple of things:

1. makes sure the command is actually useful and works
2. gives us confidence that nothing fishy is going on
3. gives us a convenient place to look for outliers (stats, log size).

I think it's fine to run this command against a running node (at least
this works like a charm locally), which is required for easy roachtest
integration. Perhaps @ajkr can confirm that this is fine.

PS I'm seeing the annoying message

> 190411 12:48:01.858427 1 storage/engine/rocksdb.go:127  [rocksdb] [/Users/tschottdorf/go/src/github.com/cockroachdb/cockroach/c-deps/rocksdb/db/version_set.cc:2566] More existing levels in DB than needed. max_bytes_for_level_multiplier may not be guaranteed.

again in this context. We need to silence that or customer support
will get asked about it all the time.

Release note: None
@tbg tbg requested a review from a team as a code owner April 11, 2019 13:48
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@tbg tbg requested a review from bobvawter April 12, 2019 09:03
Copy link
Copy Markdown
Contributor

@bobvawter bobvawter left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @tbg)


pkg/cli/debug.go, line 599 at r1 (raw file):

func (m *msgs) withPrefix(prefix string) msgs {
	mm := make(msgs, 0, len(*m))
	for _, msg := range *m {

I understand it to be slightly more efficient to avoid the append call when you know that the slice won't need to be reallocated.

mm := make(msgs, len(*m))
for idx, msg := range *m {
  msgs.s = prefix + msgs.s
  mm[idx] = msg
}

@ajkr
Copy link
Copy Markdown
Contributor

ajkr commented Apr 16, 2019

I think it's fine to run this command against a running node (at least
this works like a charm locally), which is required for easy roachtest
integration. Perhaps @ajkr can confirm that this is fine.

In general it isn't safe to open a DB in read-only mode if it's simultaneously opened in read-write mode. Compactions on the read-write instance can delete files that are part of the read-only instance's LSM, for example. I am not sure if those risks matter for this use case. But there is an upcoming safe solution to this problem to watch out for (secondary instances - facebook/rocksdb#4899).

@tbg tbg added the X-noremind Bots won't notify about PRs with X-noremind label Jun 19, 2019
@tbg tbg closed this Oct 1, 2020
@tbg tbg deleted the roachtest/run-check-store branch October 1, 2020 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

X-noremind Bots won't notify about PRs with X-noremind

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants