-
Notifications
You must be signed in to change notification settings - Fork 714
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
add capability to manually flush memtable and l0 #520
Conversation
@alxyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
storage/rocksdb/ha_rocksdb.cc
Outdated
std::vector<std::string> file_names; | ||
for (auto &file : metadata.levels[0].files) { | ||
DBUG_ASSERT(metadata.levels[0].level == 0); | ||
file_names.emplace_back(file.db_path + "/" + file.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems odd that db_path doesn't end in '/', but I do see code in rocksdb adding '/' between path and filename in some cases. I assume the "/" is required to assemble the full path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems so. I used https://github.com/facebook/rocksdb/blob/master/db/compact_files_test.cc#L105 as my reference which is doing pretty much the exact same thing we want here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I stepped through in GDB and found that actually the "/" isn't required (even though the test case above uses it).
It parses the number from the sst file during CompactFiles() using https://github.com/facebook/rocksdb/blob/master/db/filename.cc#L99-L108.
So I'll remove the extra "/" even though it doesn't hurt really
storage/rocksdb/ha_rocksdb.cc
Outdated
if (file_names.size() > 0) { | ||
rocksdb::Status s; | ||
s = rdb->CompactFiles(rocksdb::CompactionOptions(), cf_handle, file_names, | ||
metadata.levels.size() - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this target level 1 for the output, instead of the bottommost level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, no problem.
@alxyang updated the pull request - view changes - changes since last import |
@alxyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
storage/rocksdb/ha_rocksdb.cc
Outdated
file_names.emplace_back(file.db_path + file.name); | ||
} | ||
|
||
if (file_names.size() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - I have small preference for if (!file_names.empty()) {
. Not for performance - I just think it is easier to read.
storage/rocksdb/ha_rocksdb.cc
Outdated
std::vector<std::string> file_names; | ||
for (auto &file : metadata.levels[0].files) { | ||
DBUG_ASSERT(metadata.levels[0].level == 0); | ||
file_names.emplace_back(file.db_path + file.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is file.db_path
guaranteed to end with a '/'? The reason I ask is RocksDB's own tests add a '/' in the middle. https://github.com/facebook/rocksdb/blob/master/db/compact_files_test.cc#L104-L107
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally I did put a /
there. @hermanlee commented above asking if it was necessary, I stepped through GDB and found that it was not. See my comment above for more details.
See https://github.com/facebook/rocksdb/blob/master/db/filename.cc#L99-L108 for how the sstfile name is parsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. As long as it has been checked.
storage/rocksdb/ha_rocksdb.cc
Outdated
static int rocksdb_force_flush_memtable_and_lzero_now( | ||
THD *const thd, struct st_mysql_sys_var *const var, void *const var_ptr, | ||
struct st_mysql_value *const value) { | ||
sql_print_information("RocksDB: Manual memtable and L0 flush\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sql_print_information()
function automatically adds a '\n' to the end of the string that you pass it. Unless you are intentionally trying to add an empty line in the log file after this line you should remove the '\n' here. Note that rocksdb_force_flush_memtable_now()
should probably be changed as well.
storage/rocksdb/ha_rocksdb.cc
Outdated
|
||
std::vector<std::string> file_names; | ||
for (auto &file : metadata.levels[0].files) { | ||
DBUG_ASSERT(metadata.levels[0].level == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this assert inside the inner for loop? I would expect that checking this once in the outer for loop would be sufficient.
storage/rocksdb/ha_rocksdb.cc
Outdated
|
||
if (file_names.size() > 0) { | ||
rocksdb::Status s; | ||
s = rdb->CompactFiles(rocksdb::CompactionOptions(), cf_handle, file_names, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that rocksdb::CompactionOptions()
will not change throughout both the inner and the outer loop. Can we put this into a local variable to avoid calling it over and over?
storage/rocksdb/ha_rocksdb.cc
Outdated
const Rdb_cf_manager &cf_manager = rdb_get_cf_manager(); | ||
for (const auto &cf_handle : cf_manager.get_all_cf()) { | ||
rocksdb::ColumnFamilyMetaData metadata; | ||
rdb->GetColumnFamilyMetaData(cf_handle, &metadata); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is guaranteeing that the data inside metadata
is not being changed while we are reading it? Is this known to be safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When calling into GetColumnFamilyMetaData() the current SuperVersion is referenced and the metadata is copied over from that. When the function exits the SuperVersion is de-referenced. A SuperVersion represents an immutable state of the database (when the manifest is updated). It is possible, however, that we are not the first to request that a given L0 file be compacted. This would cause CompactFiles to error out.
It is also possible that by the time we request to have a file flushed the file has been deleted because we have de-referenced the SuperVersion from which the metadata was copied. To solve this we should hold our own superversion reference and copy over the metadata ourselves.
This still doesn't seem like an issue to me considering this is to be used for performance testing/debugging. There is no copy-race condition and CompactFiles() should gracefully handle all sanity checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested a few things, like passing in an SST file that no longer exists, or trying to compact a level that doesnt exist, and in both cases CompactFiles
handled it gracefully.
It'll write something to the logs like so:
2017-03-20 18:26:50 2306114 [Warning] MyRocks: failed to read/write in RocksDB. Error type = RDB_IO_ERROR_GENERAL, stat
us code = 4, status = Invalid argument: Specified compaction input file /000099.sst does not exist in column family def
ault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right - metadata is a copy. I should have seen that.
@alxyang updated the pull request - view changes - changes since last import |
@alxyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…lly flushing memtable and L0
@alxyang updated the pull request - view changes - changes since last import |
@alxyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. @update-submodule: rocksdb Closes facebook#520 Github Author: Alex Yang <alexyang@fb.com> Github PR Sync: {sync, type="child", parent="github", parentrepo="facebook/mysql-5.6", parentprnum="520", parentprfbid="156908684811954"} Test Plan: Imported from GitHub, without a `Test Plan:` line. Reviewers: herman, jkedgar, svcscm Reviewed By: svcscm Subscribers: svcscm, webscalesql-eng@fb.com Differential Revision: https://phabricator.intern.facebook.com/D4509351 Signature: t1:4509351:1490814923:d177f0faf2cc11c4e76afd5399343a34c152fd74
Upstream commit ID : fb-mysql-5.6.35/a44d6c287111c63d35fde0222adfebeb54d601c7 Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook/mysql-5.6#427. update-submodule: rocksdb Closes facebook/mysql-5.6#520 Differential Revision: D4509351 Pulled By: alxyang fbshipit-source-id: 3f04f3c
Upstream commit ID : fb-mysql-5.6.35/a44d6c287111c63d35fde0222adfebeb54d601c7 Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook/mysql-5.6#427. update-submodule: rocksdb Closes facebook/mysql-5.6#520 Differential Revision: D4509351 Pulled By: alxyang fbshipit-source-id: 3f04f3c
Upstream commit ID : fb-mysql-5.6.35/a44d6c287111c63d35fde0222adfebeb54d601c7 Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook/mysql-5.6#427. update-submodule: rocksdb Closes facebook/mysql-5.6#520 Differential Revision: D4509351 Pulled By: alxyang fbshipit-source-id: 3f04f3c
Upstream commit ID : fb-mysql-5.6.35/a44d6c287111c63d35fde0222adfebeb54d601c7 Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook/mysql-5.6#427. update-submodule: rocksdb Closes facebook/mysql-5.6#520 Differential Revision: D4509351 Pulled By: alxyang fbshipit-source-id: 3f04f3c
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook/mysql-5.6#427. update-submodule: rocksdb Closes facebook/mysql-5.6#520 Differential Revision: D4509351 Pulled By: alxyang fbshipit-source-id: 3f04f3c
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook/mysql-5.6#427. update-submodule: rocksdb Closes facebook/mysql-5.6#520 Differential Revision: D4509351 Pulled By: alxyang fbshipit-source-id: 3f04f3c
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at #427. update-submodule: rocksdb Closes #520 Differential Revision: D4509351 Pulled By: alxyang fbshipit-source-id: dd3c711
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at #427. update-submodule: rocksdb Closes #520 Differential Revision: D4509351 (44a522c) Pulled By: alxyang fbshipit-source-id: c5de1697c40
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 (facebook@44a522c) Pulled By: alxyang fbshipit-source-id: c5de1697c40
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 (facebook@44a522c) Pulled By: alxyang fbshipit-source-id: c5de1697c40
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 (facebook@44a522c) Pulled By: alxyang fbshipit-source-id: c5de1697c40
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 (facebook@44a522c) Pulled By: alxyang fbshipit-source-id: c5de1697c40
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 (facebook@44a522c) Pulled By: alxyang fbshipit-source-id: c5de1697c40
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 (facebook@44a522c) Pulled By: alxyang fbshipit-source-id: c5de1697c40
Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at #427. @update-submodule: rocksdb Closes #520 Differential Revision: D4509351 Pulled By: alxyang
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. @update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
…na#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook/mysql-5.6#427. @update-submodule: rocksdb Closes facebook/mysql-5.6#520 Differential Revision: D4509351 Pulled By: alxyang
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. @update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. @update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. @update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. @update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. @update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
…book#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook#427. @update-submodule: rocksdb Closes facebook#520 Differential Revision: D4509351 Pulled By: alxyang
…na#520) Summary: This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring. See more details at facebook/mysql-5.6#427. @update-submodule: rocksdb Closes facebook/mysql-5.6#520 Differential Revision: D4509351 Pulled By: alxyang
This provides an option to flush memtable and remove all files from L0, which can be useful for lessening variance in benchmarking on read-only and read-heavy tests, in cases where the memtable isn't full and/or there aren't many files in L0, and no writes are occurring.
See more details at #427.
@update-submodule: rocksdb