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

libroach: bump up rocksdb backpressure limits #41719

Merged
merged 1 commit into from
Oct 21, 2019

Conversation

dt
Copy link
Member

@dt dt commented Oct 18, 2019

System-critical writes in Cockroach, like node-liveness, just can not be
slow or they will fail, meaning that if theese rocksdb back-pressure
slowdowns ever kick in, they usually do not gradually slow traffic
until the system reaches some stable throughput equilibrium as intended,
but rather cause liveness to fail and result in sudden unavailability
-- the opposite of what they were intended to do.

Thus we are probably better off just letting the metrics they were
intended to protect -- like read-amplification or compaction debt --
stray further into unhealthy territory, than we are back-pressuring and
hastening our demise: slower reads due to elevated read-amp are still
better than no reads due to node-liveness failures (and indeed slower
reads may serve as their own backpressure as we usually need to read to
write).

Release note: None

@dt dt requested review from ajkr and petermattis October 18, 2019 13:16
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajkr and @petermattis)

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @dt and @petermattis)


c-deps/libroach/options.cc, line 256 at r1 (raw file):

  // TODO(dt): if/when we dynamically tune for bulk-ingestion, we
  // could leave this at 20 and only raise it during ingest jobs.
  options.level0_slowdown_writes_trigger = 500;

I wonder if we should set this to 999 or 1000. @ajkr?


c-deps/libroach/options.cc, line 275 at r1 (raw file):

  // these as-is and only raise / disable them during ingest.
  options.soft_pending_compaction_bytes_limit = 2048 * 1073741824ull;
  options.hard_pending_compaction_bytes_limit = 4098 * 1073741824ull;

Nit: can we write this constants as 2048ull << 30 and 4097ull << 30? My eyes are much more used to translating << 30 to GB, then remembering the exact numeric value.

Copy link
Member Author

@dt dt 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 (and 2 stale) (waiting on @ajkr and @petermattis)


c-deps/libroach/options.cc, line 256 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I wonder if we should set this to 999 or 1000. @ajkr?

makes sense -- if we climb meaningfully above the slowdown number, we're likely to remain slowed a while, and being in a slowdown of any nontrivial duration means we're as good as stopped anyway. bumped it to 950.


c-deps/libroach/options.cc, line 275 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: can we write this constants as 2048ull << 30 and 4097ull << 30? My eyes are much more used to translating << 30 to GB, then remembering the exact numeric value.

👍

Done.

@dt
Copy link
Member Author

dt commented Oct 21, 2019

are we okay with backporting this to release-19.2?

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

are we okay with backporting this to release-19.2?

I am. @ajkr any concerns?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @dt)


c-deps/libroach/options.cc, line 269 at r2 (raw file):

  // adding data directly. Additionally some system-critical writes in
  // cockroach (node-liveness), just can not be slow or they will fail and cause
  // unavilability, so back-pressuring may *cause* unavailability, instead of

s/unavilability/unavailability/g

System-critical writes in Cockroach, like node-liveness, just can not be
slow or they will fail, meaning that if theese rocksdb back-pressure
slowdowns ever kick in, they usually do not gradually slow traffic
until the system reaches some stable throughput equilibrium as intended,
but rather cause liveness to fail and result in sudden  unavailability
-- the opposite of what they were intended to do.

Thus we are probably better off just letting the metrics they were
intended to protect -- like read-amplification or compaction debt --
stray further into unhealthy territory, than we are back-pressuring and
hastening our demise: slower reads due to elevated read-amp are still
better than no reads due to node-liveness failures (and indeed slower
reads may serve as their own backpressure as we usually need to read to
write).

Release note: None
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

I am. @ajkr any concerns?

No concerns. Very curious whether this high L0 file count is pointing us to a problem with the intra-L0 picking heuristic, though.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @dt)


c-deps/libroach/options.cc, line 256 at r1 (raw file):

Previously, dt (David Taylor) wrote…

makes sense -- if we climb meaningfully above the slowdown number, we're likely to remain slowed a while, and being in a slowdown of any nontrivial duration means we're as good as stopped anyway. bumped it to 950.

Makes sense.

@dt
Copy link
Member Author

dt commented Oct 21, 2019

TFTRs!

bors r+

@dt
Copy link
Member Author

dt commented Oct 21, 2019

I think bors got lost again?

bors r+

@dt
Copy link
Member Author

dt commented Oct 21, 2019

it is just not your day, is it bors?

bors r+

craig bot pushed a commit that referenced this pull request Oct 21, 2019
41719: libroach: bump up rocksdb backpressure limits r=dt a=dt

System-critical writes in Cockroach, like node-liveness, just can not be
slow or they will fail, meaning that if theese rocksdb back-pressure
slowdowns ever kick in, they usually do not gradually slow traffic
until the system reaches some stable throughput equilibrium as intended,
but rather cause liveness to fail and result in sudden  unavailability
-- the opposite of what they were intended to do.

Thus we are probably better off just letting the metrics they were
intended to protect -- like read-amplification or compaction debt --
stray further into unhealthy territory, than we are back-pressuring and
hastening our demise: slower reads due to elevated read-amp are still
better than no reads due to node-liveness failures (and indeed slower
reads may serve as their own backpressure as we usually need to read to
write).

Release note: None

Co-authored-by: David Taylor <tinystatemachine@gmail.com>
@craig
Copy link
Contributor

craig bot commented Oct 21, 2019

Build succeeded

@craig craig bot merged commit 3b8aa8a into cockroachdb:master Oct 21, 2019
@dt dt deleted the rocks-backpressure branch October 22, 2019 01:30
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