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

kvserver: introduce Store.{logEngine,todoEngine} #96823

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

tbg
Copy link
Member

@tbg tbg commented Feb 8, 2023

Over the last few months, we have been chipping away1 at separating the
engine interactions of the KV server in preparation for optionally
running with a separate log engine for the raft state.

Plenty of work remains to be able to safely split these engines apart,
or to even be able to do it at all, but now's a good time to take stock
and syntactically "boil the ocean" by forcing all users of
Store.engine to decide between three Engine fields - all for now
backed by the same Engine - now present on Store:

  • LogEngine()
  • StateEngine()
  • TODOEngine()

The third engine mirrors the context.TODO() pattern - the "TODO"
engine indicates that work needs to be done to even arrive at an
operation that targets one of the engines specifically; or that there is
something that should be revisited about this particular usage.

With the stability period coming up, we will want to avoid large
mechanical changes in the near future, so now is a good time to get this
out of the way and to work towards a "prototype plus" of the raft log
separation for the remaining weeks.

Ideally this would culminate in a way to actually run with two engines
for experiments (ignoring crash-restart correctness issues and the
like), which would be an important milestone for the project and would
give us a reality check on the work that lies ahead to productionize
this work.

This refactor was carried out mechanically and it assigns everything
to the TODOEngine. In other words, no brains have been put into
deciding which engine to use yet. This will be done piecemeal in
smaller PRs down the line, and will help discover new issues for
the CRDB-220 epic.

Epic: CRDB-220
Release note: None

Footnotes

  1. https://github.com/cockroachdb/cockroach/pulls?q=is%3Apr+%22CRDB-220%22+is%3Amerged+created%3A%3C2023-02-08

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg force-pushed the two-engines branch 2 times, most recently from 4f087cb to 870468b Compare February 8, 2023 21:07
@tbg tbg requested a review from pav-kv February 8, 2023 21:08
pkg/kv/kvserver/store.go Outdated Show resolved Hide resolved
@tbg tbg marked this pull request as ready for review February 9, 2023 13:00
@tbg tbg requested a review from a team as a code owner February 9, 2023 13:00
@tbg tbg requested a review from a team February 9, 2023 13:00
@tbg tbg requested a review from a team as a code owner February 9, 2023 13:00
@tbg tbg requested a review from a team February 9, 2023 13:00
@tbg tbg requested review from a team as code owners February 9, 2023 13:00
@tbg tbg requested a review from a team February 9, 2023 13:00
@tbg tbg requested a review from a team as a code owner February 9, 2023 13:00
@tbg tbg requested review from benbardin and pav-kv and removed request for a team and benbardin February 9, 2023 13:00
Over the last few months, we have been chipping away[^1] at separating the
engine interactions of the KV server in preparation for optionally
running with a separate log engine for the raft state.

[^1]: https://github.com/cockroachdb/cockroach/pulls?q=is%3Apr+%22CRDB-220%22+is%3Amerged+created%3A%3C2023-02-08

Plenty of work remains to be able to safely split these engines apart,
or to even be able to do it at all, but now's a good time to take stock
and syntactically "boil the ocean" by forcing all users of
`Store.engine` to decide between three `Engine` fields - all for now
backed by the same Engine - now present on `Store`:

- `LogEngine()`
- `StateEngine()`
- `TODOEngine()`

The third engine mirrors the `context.TODO()` pattern - the "TODO"
engine indicates that work needs to be done to even arrive at an
operation that targets one of the engines specifically; or that there is
something that should be revisited about this particular usage.

With the stability period coming up, we will want to avoid large
mechanical changes in the near future, so now is a good time to get this
out of the way and to work towards a "prototype plus" of the raft log
separation for the remaining weeks.

Ideally this would culminate in a way to actually run with two engines
for experiments (ignoring crash-restart correctness issues and the
like), which would be an important milestone for the project and would
give us a reality check on the work that lies ahead to productionize
this work.

This refactor was carried out mechanically and it assigns *everything*
to the `TODOEngine`. In other words, no brains have been put into
deciding which engine to use yet. This will be done piecemeal in
smaller PRs down the line, and will help discover new issues for
the CRDB-220 epic.

Epic: CRDB-220
Release note: None
pkg/kv/kvserver/replica_raftstorage.go Show resolved Hide resolved
pkg/kv/kvserver/store.go Show resolved Hide resolved
pkg/kv/kvserver/store.go Show resolved Hide resolved
@tbg
Copy link
Member Author

tbg commented Feb 14, 2023

bors r=pavelkalinnikov

TFTR!

@craig
Copy link
Contributor

craig bot commented Feb 14, 2023

Build succeeded:

@craig craig bot merged commit 16fd101 into cockroachdb:master Feb 14, 2023
Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/kv/kvserver/store.go line 463 at r2 (raw file):

	todoEngine storage.Engine
	// logEngine is the engine holding the raft state.
	logEngine storage.Engine

I am very nervous about correctness if callers can directly interact with the storage.Engine interface, versus a more structured interface like in #88606.

Is this just a stop-gap, so that we know which caller needs what kind of functionality?

@pav-kv
Copy link
Collaborator

pav-kv commented Apr 15, 2023

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

pkg/kv/kvserver/store.go line 463 at r2 (raw file):

	todoEngine storage.Engine
	// logEngine is the engine holding the raft state.
	logEngine storage.Engine

I am very nervous about correctness if callers can directly interact with the storage.Engine interface, versus a more structured interface like in #88606.

Is this just a stop-gap, so that we know which caller needs what kind of functionality?

I'm on the same page with this concern. Pebble storage doesn't seem the right abstraction for raft logs, it's too broad. Having two engines like this can be useful for PoC performance gain demo (which, I think, was the intent behind this PR), but to make it correct I would rather be coming from a very restrictive interface fit exactly for the purpose.

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.

4 participants