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

storage: change default engine type from RocksDB to Pebble #48145

Merged
merged 2 commits into from
May 9, 2020

Conversation

petermattis
Copy link
Collaborator

This flips the switch to use Pebble by default. This will cause all tests
which don't explicitly specify RocksDB (i.e. the vast majority) to now use
Pebble. One exception is certain roachtests which create store directories
using older versions of CRDB and then upgrade to the current version. Those
tests will continue to use RocksDB (for now) due to the nature of how
--storage-engine=... is "sticky" if not specified, defaulting to the
previously used storage engine.

Release note (general change): Change the default engine type for new
storage directories from RocksDB to Pebble. Existing stores will continue
to use the previously specified storage engine, and an explicit
specification (via --storage-engine=...) will override the default.

@petermattis petermattis requested a review from a team as a code owner April 29, 2020 11:44
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis
Copy link
Collaborator Author

[The first commit is #48144].

I'm not sure we want to merge this quite yet, but putting it out there for review and to see what CI thinks of it. I still have to do a full roachtest run on this PR to make sure that no roachtest regressions have slipped in.

@blathers-crl
Copy link

blathers-crl bot commented Apr 29, 2020

❌ The GitHub CI (Cockroach) build has failed on 147cb552.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl
Copy link

blathers-crl bot commented Apr 29, 2020

❌ The GitHub CI (Cockroach) build has failed on 193ba988.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@petermattis petermattis force-pushed the pmattis/default-engine-type branch 2 times, most recently from 0dbccd3 to 3b97957 Compare April 29, 2020 13:15
@blathers-crl
Copy link

blathers-crl bot commented Apr 29, 2020

❌ The GitHub CI (Cockroach) build has failed on 3b97957d.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl
Copy link

blathers-crl bot commented Apr 29, 2020

❌ The GitHub CI (Cockroach) build has failed on 58b1de3c.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

Copy link
Member

@itsbilal itsbilal left a comment

Choose a reason for hiding this comment

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

:lgtm: when all tests pass.

PR title and commit message should have engines in the opposite order, unless we're going back in time 😂

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

@itsbilal itsbilal assigned itsbilal and unassigned itsbilal Apr 30, 2020
@petermattis
Copy link
Collaborator Author

PR title and commit message should have engines in the opposite order, unless we're going back in time.

Ha! Oops.

@petermattis petermattis changed the title storage: change default engine type from Pebble to RocksDB storage: change default engine type from RocksDB to Pebble Apr 30, 2020
@blathers-crl
Copy link

blathers-crl bot commented Apr 30, 2020

❌ The GitHub CI (Cockroach) build has failed on 1d05b844.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@petermattis
Copy link
Collaborator Author

DockerCLI acceptance failures are real. I'll send a separate PR to fix them shortly.

@petermattis petermattis force-pushed the pmattis/default-engine-type branch from 1d05b84 to cd190ae Compare May 4, 2020 21:11
@petermattis
Copy link
Collaborator Author

DockerCLI acceptance failures are real. I'll send a separate PR to fix them shortly.

Should be fixed now. I need to do another full roachtest-on-pebble run to make sure no failures have slipped in. Otherwise, this is good to go.

@blathers-crl
Copy link

blathers-crl bot commented May 4, 2020

❌ The GitHub CI (Cockroach) build has failed on cd190ae6.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@petermattis
Copy link
Collaborator Author

Filed #48413 about the CI failure which looks unrelated to this PR.

@blathers-crl
Copy link

blathers-crl bot commented May 5, 2020

❌ The GitHub CI (Cockroach) build has failed on cd190ae6.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl
Copy link

blathers-crl bot commented May 5, 2020

❌ The GitHub CI (Cockroach) build has failed on cd190ae6.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@petermattis petermattis force-pushed the pmattis/default-engine-type branch from cd190ae to c0da218 Compare May 5, 2020 19:44
@blathers-crl
Copy link

blathers-crl bot commented May 5, 2020

❌ The GitHub CI (Cockroach) build has failed on c0da218d.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl
Copy link

blathers-crl bot commented May 6, 2020

❌ The GitHub CI (Cockroach) build has failed on 3d63dc1a.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@petermattis petermattis force-pushed the pmattis/default-engine-type branch from 3d63dc1 to 3506ed1 Compare May 6, 2020 17:26
Copy link
Member

@itsbilal itsbilal left a comment

Choose a reason for hiding this comment

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

Is that a green checkmark I see? :shipit:

:lgtm_strong:

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jbowens)

@petermattis
Copy link
Collaborator Author

Is that a green checkmark I see? :shipit:

Almost time. I still need to do another roachtest-on-Pebble run.

This flips the switch to use Pebble by default. This will cause all
tests which don't explicitly specify RocksDB (i.e. the vast majority) to
now use Pebble. One exception is certain roachtests which create store
directories using older versions of CRDB and then upgrade to the current
version. Those tests will continue to use RocksDB (for now) due to the
nature of how `--storage-engine=...` is "sticky" if not specified,
defaulting to the previously used storage engine.

Release note (general change): Change the default engine type for new
storage directories from RocksDB to Pebble. Existing stores will
continue to use the previously specified storage engine, and an explicit
specification (via `--storage-engine=...`) will override the default.
@petermattis petermattis force-pushed the pmattis/default-engine-type branch from 3506ed1 to 5d08f59 Compare May 9, 2020 18:07
@petermattis
Copy link
Collaborator Author

I still need to do another roachtest-on-Pebble run.

256 PASS, 12 FAIL. 1 of the failures is real ("synctest" is now skipped, see #48603). 3 of the failures were due to test infrastructure problems. The remaining 8 failures are flaky tests that periodically fail on RocksDB as well. I looked at all of the failures and nothing Pebble-specific jumped out about them.

I'm going to merge this when CI is green.

@petermattis
Copy link
Collaborator Author

TFTR! 🤞

bors r+

@craig
Copy link
Contributor

craig bot commented May 9, 2020

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented May 9, 2020

Build succeeded

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.

3 participants