RemoteCompaction support Fallback to local compaction #8709
Conversation
|
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| start_str = Key(120); | ||
| start = start_str; | ||
| comp_num = my_cs->GetCompactionNum(); | ||
| ASSERT_OK(db_->CompactRange(CompactRangeOptions(), &start, nullptr)); |
There was a problem hiding this comment.
Do we expect some stat to have increased on primary side , like COMPACTION_BYTES_READ?
There was a problem hiding this comment.
I added check for statistics->getTickerCount(COMPACTION_KEY_DROP_NEWER_ENTRY), which shows the updated keys.
Not sure how we could check COMPACTION_BYTES_READ, seems we need to do GetThreadList() and check?
There was a problem hiding this comment.
Sounds good, thanks. COMPACTION_BYTES_READ is also a stat you can get from statistics->getTickerCount().
Is this the fix?
ASSERT_GE(primary_statistics->getTickerCount(COMPACTION_KEY_DROP_NEWER_ENTRY),
primary_new_key);
If no compaction happened on the primary would that still pass because the ticker stat would equal primary_new_key?
There was a problem hiding this comment.
my mistake, it should ASSERT_GT not GE.
There was a problem hiding this comment.
Sorry for the confusion. COMPACT_READ_BYTES is the name of the ticker stat as we discussed.
There was a problem hiding this comment.
make sense.
But currently, we record the compact_read/write_bytes on both remote and local side (which should be fixed):
https://github.com/facebook/rocksdb/blob/9dd09dc4f302f2de5a2c34ab2cbc4c393b4503cc/db/compaction/compaction_job.cc#L1117-L1118
I think we did that at the beginning just to make sure with/without remote compaction it behaves exactly the same, which it's not necessary. I'll fix that and maybe add new metrics for remote compaction if we need record that.
On the other hand, I think because of the how the test CompactionService is implemented (which runs the remote compaction with the local compaction thread), the read/write bytes is still counted locally. I think we may need to update the test CompactionService, maybe to a separate process (ideally).
Anyway, I'm not going to fix it in this diff. Will follow up on that.
db/compaction/compaction_job.cc
Outdated
| ROCKS_LOG_WARN(db_options_.info_log, | ||
| "[%s] [JOB %d] Remote compaction failed, status: %s", | ||
| compaction_input.column_family.name.c_str(), job_id_, | ||
| s.ToString().c_str()); |
There was a problem hiding this comment.
This feels like tricky edge cases. If remote compaction returns kFailure and we can parse the compaction_result and it has compaction_result.status.ok(), then sub_compact->status will be OK. Is that right?
Logging s also could be confusing because it's the parsing status not the remote compaction status.
There was a problem hiding this comment.
Yeah, I think that could happen, added log for such case.
And remove logging parsing status.
jay-zhuang
force-pushed
the
rc_runlocal
branch
from
136ba2f
to
9e6daec
Compare
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
jay-zhuang
force-pushed
the
rc_runlocal
branch
from
9e6daec
to
b8f94f6
Compare
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
|
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| start_str = Key(120); | ||
| start = start_str; | ||
| comp_num = my_cs->GetCompactionNum(); | ||
| ASSERT_OK(db_->CompactRange(CompactRangeOptions(), &start, nullptr)); |
There was a problem hiding this comment.
Sounds good, thanks. COMPACTION_BYTES_READ is also a stat you can get from statistics->getTickerCount().
Is this the fix?
ASSERT_GE(primary_statistics->getTickerCount(COMPACTION_KEY_DROP_NEWER_ENTRY),
primary_new_key);
If no compaction happened on the primary would that still pass because the ticker stat would equal primary_new_key?
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
|
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
jay-zhuang
force-pushed
the
rc_runlocal
branch
from
dca9b47
to
66beb91
Compare
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
|
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
|
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Add support for fallback to local compaction, the user can return `CompactionServiceJobStatus::kUseLocal` to instruct RocksDB to run the compaction locally instead of waiting for the remote compaction result. Test Plan: unittest
jay-zhuang
force-pushed
the
rc_runlocal
branch
from
865f4cc
to
6fa2ecf
Compare
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
|
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@jay-zhuang merged this pull request in 1c290c7. |
Summary: Add support for fallback to local compaction, the user can return `CompactionServiceJobStatus::kUseLocal` to instruct RocksDB to run the compaction locally instead of waiting for the remote compaction result. Pull Request resolved: facebook/rocksdb#8709 Test Plan: unittest Reviewed By: ajkr Differential Revision: D30560163 Pulled By: jay-zhuang fbshipit-source-id: 65d8905a4a1bc185a68daa120997f21d3198dbe1
Summary: Add support for fallback to local compaction, the user can
return
CompactionServiceJobStatus::kUseLocalto instruct RocksDB torun the compaction locally instead of waiting for the remote compaction
result.
Test Plan: unittest