Some API clarifications #9080

pdillinger · 2021-10-26T23:49:25Z

Summary:

Clarify that RocksDB is not exception safe on many of our callback
and extension interfaces
Clarify FSRandomAccessFile::MultiRead implementations must accept
non-sorted inputs (see Sort per-file blob read requests by offset #8953)
Clarify ConcurrentTaskLimiter and SstFileManager are not (currently)
extensible interfaces
Mark WriteBufferManager as final, so it is then clearly not a
callback interface, even though it smells like one
Clarify TablePropertiesCollector Status returns are mostly ignored

Test Plan: comments only (except WriteBufferManager final)

Summary: * Clarify FSRandomAccessFile::MultiRead implementations must accept non-sorted inputs * Clarify ConcurrentTaskLimiter and SstFileManager are not callback interfaces * Mark WriteBufferManager as `final`, so it is then clearly not a callback interface, even though it smells like one * Clarify TablePropertiesCollector Status returns are mostly ignored * Clarify that RocksDB is not exception safe on many of our callback interfaces Test Plan: comments only (except WriteBufferManager final)

facebook-github-bot · 2021-10-27T19:15:57Z

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mrambacher · 2021-10-28T22:26:55Z

include/rocksdb/compaction_filter.h

+//
+// RocksDB callbacks are NOT exception-safe. A callback completing with an
+// exception can lead to undefined behavior in RocksDB, including data loss,
+// unreported corruption, deadlocks, and more.


Most of these classes do not define "callbacks" per-se (at least in the classical sense) but methods that can be overridden.

Can we define somewhere more generally (like in programming rules or extensions) that RocksDB is not exception safe, rather than mentioning it for every class. Seems repetitive.

This website I found agrees with your terminology. How about this:

// Exceptions MUST NOT propagate out of overridden functions into RocksDB, // because RocksDB is not exception-safe. This could cause undefined behavior // including data loss, unreported corruption, deadlocks, and more.

(Technically people can do whatever they want in overrides; it's only a problem if they provide their derived implementations back to RocksDB such that RocksDB will call into their implementation.)

And @ajkr has noted in the past that some RocksDB applications prefer a chance of persisting relevant information to avoiding corruption. I think that's a small minority of applicatoins and they should be able to make their own judgment despite the MUST NOT warning.

rather than mentioning it for every class

Debugging corruption is a nightmare. More often we bear the investigative consequences of violated API contracts because the problem presents as a RocksDB problem. To me it's clearly worthwhile to make non-obvious and high-impact contracts clear everywhere they are relevant, to minimize the potential for violations. We do not live in an ideal world in which everyone using RocksDB reads and absorbs all available documentation before writing code. Even if we did, we now need to "correct the record," because this issue is arguably mis-documented. AFAIK, we only mention exceptions here and even subtly suggest that exceptions passing through RocksDB is OK. (I will update the wiki after this PR lands.) And I know from experience that people don't read release notes, so release note+wiki is not sufficient.

This website I found agrees with your terminology. How about this:

// Exceptions MUST NOT propagate out of overridden functions into RocksDB, // because RocksDB is not exception-safe. This could cause undefined behavior // including data loss, unreported corruption, deadlocks, and more.

How about: "RocksDB is not exception-safe. Exceptions MUST not propagate into RocksDB or undefined behavior -- including data loss, unreported corruption, and deadlocks -- may occur"

rather than mentioning it for every class

Debugging corruption is a nightmare. More often we bear the investigative consequences of violated API contracts because the problem presents as a RocksDB problem. To me it's clearly worthwhile to make non-obvious and high-impact contracts clear everywhere they are relevant, to minimize the potential for violations. We do not live in an ideal world in which everyone using RocksDB reads and absorbs all available documentation before writing code. Even if we did, we now need to "correct the record," because this issue is arguably mis-documented. AFAIK, we only mention exceptions here and even subtly suggest that exceptions passing through RocksDB is OK. (I will update the wiki after this PR lands.) And I know from experience that people don't read release notes, so release note+wiki is not sufficient.

I agree with you about the difficulty in debugging corruptions. But I am not sure how this documentation will make it more likely that others read the documentation and understood it and any problems are not related to this issue.

I also worry that there may be other things (mis-matched compiler options, RTTI, alignment issues, etc) that might cause similar issues in the future and would also need to be documented everywhere. Having a single place that outlined the rules for extending RocksDB would make it easier to maintain the documentation and a consistent message.

// Exceptions MUST NOT propagate out of overridden functions into RocksDB,
// because RocksDB is not exception-safe. This could cause undefined behavior
// including data loss, unreported corruption, deadlocks, and more.

This works for me.

Let's suppose two different people involved in application X built on RocksDB do the following: Person 1 handles the build integration and Person 2 writes source code using and extending RocksDB. What are the "other things" Person 2 needs to know about that specifically relate to RocksDB? I can't think of how any you have listed apply to Person 2 specifically in their interaction with RocksDB. Limitations due to compiler options, RTTI availability, etc. will apply project-wide. Your suggestions seem irrelevant at the source code API level. If you have a specific example of a piece of contract that you think is equatable with mine, please share.

mrambacher · 2021-10-28T22:28:36Z

include/rocksdb/concurrent_task_limiter.h

+// This is NOT a callback interface but a public interface for result of
+// NewConcurrentTaskLimiter. Any derived classes must be RocksDB internal.


Is this really true? The user cannot define their own implementation? Why not? If so, then the interface here should/could be rethought/different.

rocksdb/db/db_impl/db_impl_compaction_flush.cc

Lines 68 to 69 in f24c39a

auto limiter = static_cast<ConcurrentTaskLimiterImpl*>(

cfd->ioptions()->compaction_thread_limiter.get());

If there is only this one use, it seems like an easy one to actually fix to allow multiple implementations.

OK, we can fix this my putting GetToken in the public API.

OK, we can fix this my putting GetToken in the public API.

What function might be implemented differently from ConcurrentTaskLimiterImpl?

I don't really care whether it's extensible or not. We could idiomatically enforce "internal extension only" with private ctor and friend the Impl class.

In either case, I could envision someone wanting public access to GetToken, just as people use our RateLimiter to rate limit other things along with RocksDB.

Non-trivial issue because of the Token class. Postponing to another PR.

mrambacher · 2021-10-28T22:35:55Z

include/rocksdb/sst_file_manager.h

-// SstFileManager is not extensible.
+// SstFileManager is NOT a callback interface but a public interface for
+// result of NewSstFileManager. Any derived classes must be RocksDB internal.


Again, I am not sure why this is true. I know the RocksDB Cloud attempts to extend it

$ git grep static_cast.SstFileManagerImpl | grep -v test.c | wc -l 13 $

Rather than internal, can we say must extend the internal Impl class?

I still hate this limit/requirement. Seems like a bad design...

Given the number of non-override functions in the Impl class, this seems much harder to untangle

Agreed.

I know that the cloud folks extended the Impl one, which appears to be the way one would have to go currently (which is not great but...). I am not sure about the "RocksDB internal" wording in the comment. This is one of those gray areas where the code/documentation/design do not line up. It looks like this class should be replaceable but in reality it is not. But to extend it requires access to something that is not public. And we know of scenarios where users felt the need to extend it.

Ideas?

ajkr

LGTM

ajkr · 2021-10-29T21:59:04Z

include/rocksdb/compaction_filter.h

+//
+// RocksDB callbacks are NOT exception-safe. A callback completing with an
+// exception can lead to undefined behavior in RocksDB, including data loss,
+// unreported corruption, deadlocks, and more.


// Exceptions MUST NOT propagate out of overridden functions into RocksDB,
// because RocksDB is not exception-safe. This could cause undefined behavior
// including data loss, unreported corruption, deadlocks, and more.

This works for me.

ajkr · 2021-10-29T22:05:07Z

include/rocksdb/concurrent_task_limiter.h

+// This is NOT a callback interface but a public interface for result of
+// NewConcurrentTaskLimiter. Any derived classes must be RocksDB internal.


OK, we can fix this my putting GetToken in the public API.

What function might be implemented differently from ConcurrentTaskLimiterImpl?

hx235 · 2021-11-01T23:01:15Z

include/rocksdb/write_buffer_manager.h

@@ -34,7 +34,7 @@ class StallInterface {
  virtual void Signal() = 0;
 };

-class WriteBufferManager {
+class WriteBufferManager final {


I like this! One of my previous PR had this assumption in mind that WBM should not be extended and made an API implement change in here. It's good to explicitly mark it final just to confirm my assumption!

facebook-github-bot · 2021-11-02T17:41:32Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-11-02T18:09:22Z

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-11-02T22:31:41Z

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-11-03T03:30:18Z

@pdillinger merged this pull request in 82afa01.

facebook-github-bot added the CLA Signed label Oct 26, 2021

pdillinger requested review from ajkr and mrambacher October 28, 2021 19:53

mrambacher reviewed Oct 28, 2021

View reviewed changes

ajkr approved these changes Oct 29, 2021

View reviewed changes

hx235 reviewed Nov 1, 2021

View reviewed changes

pdillinger added 3 commits November 2, 2021 09:40

Merge remote-tracking branch 'origin/main' into api_clarification

fc20025

Fix HISTORY

c92bf46

Different language for extension vs. callback interfaces

2f60b33

facebook-github-bot closed this in 82afa01 Nov 3, 2021

facebook-github-bot added the Merged label Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some API clarifications #9080

Some API clarifications #9080

pdillinger commented Oct 26, 2021 •

edited

Loading

facebook-github-bot commented Oct 27, 2021

mrambacher Oct 28, 2021

pdillinger Oct 29, 2021

mrambacher Oct 29, 2021

ajkr Oct 29, 2021

pdillinger Oct 29, 2021 •

edited

Loading

mrambacher Oct 28, 2021

pdillinger Oct 28, 2021

mrambacher Oct 28, 2021

pdillinger Oct 29, 2021

ajkr Oct 29, 2021

pdillinger Oct 29, 2021

pdillinger Nov 2, 2021

mrambacher Oct 28, 2021

pdillinger Oct 28, 2021

mrambacher Oct 28, 2021

pdillinger Oct 29, 2021

mrambacher Oct 29, 2021

ajkr left a comment

ajkr Oct 29, 2021

ajkr Oct 29, 2021

hx235 Nov 1, 2021 •

edited

Loading

facebook-github-bot commented Nov 2, 2021

facebook-github-bot commented Nov 2, 2021

facebook-github-bot commented Nov 2, 2021

facebook-github-bot commented Nov 3, 2021

		// This is NOT a callback interface but a public interface for result of
		// NewConcurrentTaskLimiter. Any derived classes must be RocksDB internal.

	auto limiter = static_cast<ConcurrentTaskLimiterImpl*>(
	cfd->ioptions()->compaction_thread_limiter.get());

Some API clarifications #9080

Some API clarifications #9080

Conversation

pdillinger commented Oct 26, 2021 • edited Loading

facebook-github-bot commented Oct 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdillinger Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hx235 Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Nov 2, 2021

facebook-github-bot commented Nov 2, 2021

facebook-github-bot commented Nov 2, 2021

facebook-github-bot commented Nov 3, 2021

pdillinger commented Oct 26, 2021 •

edited

Loading

pdillinger Oct 29, 2021 •

edited

Loading

hx235 Nov 1, 2021 •

edited

Loading