Add PathNotFound subcode to IOError by riversand963 · Pull Request #4745 · facebook/rocksdb

riversand963 · 2018-12-04T02:01:17Z

Summary: As titled. Returning error about non-existing path can help user
better handle them.

Test Plan:

$make clean && make -j32 all check

Reviewers:

Subscribers:

Tasks:

Tags:

facebook-github-bot

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

facebook-github-bot · 2018-12-04T17:45:16Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

sagar0 · 2018-12-04T18:15:00Z

Don't we already use kNotFound for this specific case? So, why do we need a new one?

The Code::kNotFound is intended to denote key-value pair is not found. File not found is an IOError and should be represented with the combination of IOError and a proper subcode. Let me know if I have missed any background context on the meaning of kNotFound.

I said that because we use Status::NotFound() at various places when combined with Env::FileExists() check too.

rocksdb/include/rocksdb/env.h

Lines 225 to 230 in 2f1ca4e

// Returns OK if the named file exists.

// NotFound if the named file does not exist,

// the calling process does not have permission to determine

// whether this file exists, or if the path is invalid.

// IOError if an IO Error was encountered

virtual Status FileExists(const std::string& fname) = 0;

Oh this makes sense. Thanks!

Chatted offline with @siying . Opening a non-existing file for read is an IOError.
TableCache should return IOError instead of NotFound when the file does not exist. Therefore, we still need a new subcode.

sagar0 · 2018-12-04T18:16:22Z

May be these instances could be changed to Status::NotFound() ?

anand1976

Is Status::Corruption() a more appropriate error in this case? That's what we return if we're unable to open a file during consistency check at DB open time

riversand963 · 2018-12-04T19:08:02Z

@anand1976, my understanding is that if open (POSIX) fails and errno == ENOENT, it means the file does not exist, but file system can still be consistent.

anand1976 · 2018-12-04T19:20:08Z

@riversand963 Yes, the file system is consistent. The DB, however, is inconsistent. I guess I'm a little concerned about overloading IOError, since ideally IOError should be for hardware errors (or atleast limit it to EIO error from the underlying fs).

facebook-github-bot · 2018-12-05T23:41:02Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

sagar0 · 2018-12-07T21:36:16Z

@riversand963 "File not found" error should be handled differently at different layers. For example:

if its at the env level, asking a file system to check for the existence of a file should definitely return "not found" imo, when the file doesn't exist.
if its a DB internal check trying to open a non-existent file when the DB thinks it should exist, then there is definitely something wrong (a bug? a corruption? an hw-issue?) and we should respond appropriately.

I believe it depends on the specific case we are trying to address.

At Which level do you want to use this? I didn't get a good sense of it from the description or the modified windows code.

riversand963 · 2018-12-07T22:51:15Z

@riversand963 "File not found" error should be handled differently at different layers. For example:

if its at the env level, asking a file system to check for the existence of a file should definitely return "not found" imo, when the file doesn't exist.

Agree.

if its a DB internal check trying to open a non-existent file when the DB thinks it should exist, then there is definitely something wrong (a bug? a corruption? an hw-issue?) and we should respond appropriately.

Can be the case in which RocksDB secondary tails the MANIFEST file and updates its version storage info. Then the secondary instance tries to access a file as indicated by version storage info, but the file may have been deleted. This is not a bug, nor a corruption or hw-issue. It's just because there can be a gap between the secondary reading the MANIFEST and accessing the SST. We need a different error so that RocksDB can tail more from the MANIFEST.

I believe it depends on the specific case we are trying to address.

At Which level do you want to use this? I didn't get a good sense of it from the description or the modified windows code.

facebook-github-bot · 2018-12-18T00:59:21Z

@riversand963 has updated the pull request. Re-import the pull request

Summary: As titled. Returning error about non-existing path can help user better handle them. Test Plan: ``` $make clean && make -j32 all check ``` Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2019-01-08T19:49:57Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

riversand963 · 2019-01-18T18:52:35Z

Moved to #4899

Summary: This PR allows RocksDB to run in single-primary, multi-secondary process mode. The writer is a regular RocksDB (e.g. an `DBImpl`) instance playing the role of a primary. Multiple `DBImplSecondary` processes (secondaries) share the same set of SST files, MANIFEST, WAL files with the primary. Secondaries tail the MANIFEST of the primary and apply updates to their own in-memory state of the file system, e.g. `VersionStorageInfo`. This PR has several components: 1. (Originally in #4745). Add a `PathNotFound` subcode to `IOError` to denote the failure when a secondary tries to open a file which has been deleted by the primary. 2. (Similar to #4602). Add `FragmentBufferedReader` to handle partially-read, trailing record at the end of a log from where future read can continue. 3. (Originally in #4710 and #4820). Add implementation of the secondary, i.e. `DBImplSecondary`. 3.1 Tail the primary's MANIFEST during recovery. 3.2 Tail the primary's MANIFEST during normal processing by calling `ReadAndApply`. 3.3 Tailing WAL will be in a future PR. 4. Add an example in 'examples/multi_processes_example.cc' to demonstrate the usage of secondary RocksDB instance in a multi-process setting. Instructions to run the example can be found at the beginning of the source code. Pull Request resolved: #4899 Differential Revision: D14510945 Pulled By: riversand963 fbshipit-source-id: 4ac1c5693e6012ad23f7b4b42d3c374fecbe8886

Summary: This PR allows RocksDB to run in single-primary, multi-secondary process mode. The writer is a regular RocksDB (e.g. an `DBImpl`) instance playing the role of a primary. Multiple `DBImplSecondary` processes (secondaries) share the same set of SST files, MANIFEST, WAL files with the primary. Secondaries tail the MANIFEST of the primary and apply updates to their own in-memory state of the file system, e.g. `VersionStorageInfo`. This PR has several components: 1. (Originally in facebook#4745). Add a `PathNotFound` subcode to `IOError` to denote the failure when a secondary tries to open a file which has been deleted by the primary. 2. (Similar to facebook#4602). Add `FragmentBufferedReader` to handle partially-read, trailing record at the end of a log from where future read can continue. 3. (Originally in facebook#4710 and facebook#4820). Add implementation of the secondary, i.e. `DBImplSecondary`. 3.1 Tail the primary's MANIFEST during recovery. 3.2 Tail the primary's MANIFEST during normal processing by calling `ReadAndApply`. 3.3 Tailing WAL will be in a future PR. 4. Add an example in 'examples/multi_processes_example.cc' to demonstrate the usage of secondary RocksDB instance in a multi-process setting. Instructions to run the example can be found at the beginning of the source code. Pull Request resolved: facebook#4899 Differential Revision: D14510945 Pulled By: riversand963 fbshipit-source-id: 4ac1c5693e6012ad23f7b4b42d3c374fecbe8886

riversand963 self-assigned this Dec 4, 2018

riversand963 requested review from anand1976 and siying December 4, 2018 02:01

facebook-github-bot added the CLA Signed label Dec 4, 2018

facebook-github-bot reviewed Dec 4, 2018

View reviewed changes

riversand963 force-pushed the file_not_exist_err branch from 87cce85 to d110b6c Compare December 4, 2018 17:45

facebook-github-bot reviewed Dec 4, 2018

View reviewed changes

sagar0 reviewed Dec 4, 2018

View reviewed changes

Comment thread port/win/io_win.h Outdated

Copy link
Copy Markdown

Contributor

sagar0 Dec 4, 2018

Choose a reason for hiding this comment

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

May be these instances could be changed to Status::NotFound() ?

anand1976 reviewed Dec 4, 2018

View reviewed changes

riversand963 force-pushed the file_not_exist_err branch from d110b6c to b680212 Compare December 5, 2018 23:40

facebook-github-bot reviewed Dec 5, 2018

View reviewed changes

riversand963 force-pushed the file_not_exist_err branch from b680212 to e09196f Compare December 18, 2018 00:59

riversand963 mentioned this pull request Dec 20, 2018

Add support for MANIFEST tailing #4710

Closed

Add PathNotFound subcode to IOError

6034b95

Summary: As titled. Returning error about non-existing path can help user better handle them. Test Plan: ``` $make clean && make -j32 all check ``` Reviewers: Subscribers: Tasks: Tags:

riversand963 force-pushed the file_not_exist_err branch from e09196f to 6034b95 Compare January 8, 2019 19:49

facebook-github-bot reviewed Jan 8, 2019

View reviewed changes

riversand963 mentioned this pull request Jan 18, 2019

Support for single-primary, multi-secondary instances #4899

Closed

riversand963 closed this Jan 18, 2019

riversand963 mentioned this pull request Jan 29, 2019

[ABANDON] Support for single-primary, multi-secondary processes #4931

Closed

	// Returns OK if the named file exists.
	// NotFound if the named file does not exist,
	// the calling process does not have permission to determine
	// whether this file exists, or if the path is invalid.
	// IOError if an IO Error was encountered
	virtual Status FileExists(const std::string& fname) = 0;

Conversation

riversand963 commented Dec 4, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 4, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

sagar0 Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

riversand963 Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

sagar0 Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riversand963 Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

riversand963 Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

sagar0 Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

anand1976 left a comment

Choose a reason for hiding this comment

Uh oh!

riversand963 commented Dec 4, 2018

Uh oh!

anand1976 commented Dec 4, 2018

Uh oh!

facebook-github-bot commented Dec 5, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

sagar0 commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riversand963 commented Dec 7, 2018

Uh oh!

facebook-github-bot commented Dec 18, 2018

Uh oh!

facebook-github-bot commented Jan 8, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

riversand963 commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sagar0 Dec 4, 2018 •

edited

Loading

sagar0 commented Dec 7, 2018 •

edited

Loading