Make FileType Public and Replace kLogFile with kWalFile #7580
Conversation
include/rocksdb/options.h
Outdated
| @@ -57,6 +57,21 @@ class FileSystem; | |||
| struct Options; | |||
| struct DbPath; | |||
|
|
|||
| // To indicate the file types | |||
| enum FileType { | |||
There was a problem hiding this comment.
Since this is not public, can you add a brief comment as to what all of these mean?
Also, should there be a kUnknownFile for types that we do not know about?
There was a problem hiding this comment.
Sure. Totally agree, some of the name does not directly indicate the file type. I will add the comments. Since I move it from file_name.h to options.h, I prefer not to add kUnknownFile at this moment. In the future, if we have the use case, we can add it easily.
include/rocksdb/options.h
Outdated
| kCurrentFile, | ||
| kTempFile, | ||
| kInfoLogFile, // Either the current one, or an old one | ||
| kMetaDatabase, |
There was a problem hiding this comment.
What is a "kMetaDatabase"? Is this something that has not been developed or is it dead code? Not sure if this belongs in the public API...
There was a problem hiding this comment.
FileType is touching implementation details, so it is definitely of the "advanced options" flavor. Users should rarely need FileType. Even if checksum_handoff_file_types belongs in DBOptions, can we put this in some header included by options.h? Or we could even consider a forward declaration in options.h. If we get the enum definition out of options.h, I don't think it's so critical to get clear comments on all the enumerators. This enum is for advanced usage.
I think we generally prefer 'enum class' for new enums (or in this case, newly-public enums). What do you think?
include/rocksdb/options.h
Outdated
| @@ -57,6 +57,21 @@ class FileSystem; | |||
| struct Options; | |||
| struct DbPath; | |||
|
|
|||
| // To indicate the file types | |||
There was a problem hiding this comment.
Perhaps
// The types of files RocksDB uses in a DB directory. (Available for advanced options.)
There was a problem hiding this comment.
Perhaps the enum should be defined in "advanced_options.h" instead of options.h?
I am not in favor of classes that are defined in the public API only having forward declarations.
There was a problem hiding this comment.
If we want to expose file type as public, how about considering the file "include/rocksdb/types.h"? According to the file's comment, it is for all public custom types.
There was a problem hiding this comment.
Sure, I will move to "include/rocksdb/types.h", it makes more sense for me.
include/rocksdb/options.h
Outdated
| @@ -57,6 +57,21 @@ class FileSystem; | |||
| struct Options; | |||
| struct DbPath; | |||
|
|
|||
| // To indicate the file types | |||
There was a problem hiding this comment.
If we want to expose file type as public, how about considering the file "include/rocksdb/types.h"? According to the file's comment, it is for all public custom types.
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@zhichao-cao merged this pull request in d8ec0a7. |
Summary: As suggested by pdillinger ,The name of kLogFile is misleading, in some tests, kLogFile is defined as info log. Replace it with kWalFile and move it to public, which will be used in facebook#7523 Pull Request resolved: facebook#7580 Test Plan: make check Reviewed By: riversand963 Differential Revision: D24485420 Pulled By: zhichao-cao fbshipit-source-id: 955e3dacc1021bb590fde93b0a568ffe9ad80799
Summary: As suggested by pdillinger ,The name of kLogFile is misleading, in some tests, kLogFile is defined as info log. Replace it with kWalFile and move it to public, which will be used in facebook/rocksdb#7523 Pull Request resolved: facebook/rocksdb#7580 Test Plan: make check Reviewed By: riversand963 Differential Revision: D24485420 Pulled By: zhichao-cao fbshipit-source-id: 955e3dacc1021bb590fde93b0a568ffe9ad80799 Signed-off-by: Changlong Chen <levisonchen@live.cn>
Summary: As suggested by pdillinger ,The name of kLogFile is misleading, in some tests, kLogFile is defined as info log. Replace it with kWalFile and move it to public, which will be used in facebook/rocksdb#7523 Pull Request resolved: facebook/rocksdb#7580 Test Plan: make check Reviewed By: riversand963 Differential Revision: D24485420 Pulled By: zhichao-cao fbshipit-source-id: 955e3dacc1021bb590fde93b0a568ffe9ad80799 Signed-off-by: Changlong Chen <levisonchen@live.cn>
As suggested by @pdillinger ,The name of kLogFile is misleading, in some tests, kLogFile is defined as info log. Replace it with kWalFile and move it to public, which will be used in #7523
Test plan: make check