rocksdb: Upgrade (bump) rocksdb to latest facebook's v7.9.2#49006
rocksdb: Upgrade (bump) rocksdb to latest facebook's v7.9.2#49006
Conversation
|
@yanghonggang very interesting! We'll need to run through testing, but facebook/rocksdb#9423 sounds like a good rationale to upgrade. |
|
@markhpc with #47221 merged, in light of our production findings, we would be very interested in this moving forward since it may address some of the performance impact (at least some stalled writes) when compactions are running behind. Just reading through the RocksDB change log, there's quite a few others that sound like they might be nice (just one of many examples): 7.7.0:
It looks like 7.8.3 is out, and there have been some bugfixes since 7.7.3, so I do suggest that the PR be updated to reflect the newer version if we're going to spend the effort testing/validating safety/performance. |
2aa69ba to
f615ccd
Compare
|
If we don't make ceph-specific modifications to rocksdb, |
|
@yanghonggang We discussed this at the performance meeting last week. I think we definitely want to get this into testing for Reef. We are still recovering from an infrastructure failure in the lab, but hopefully we can get this into testing soon. |
markhpc
left a comment
There was a problem hiding this comment.
Unfortunately we can't switch over to RocksDB's repo. We still need this commit for our builds to fix a bug that was introduced several years ago:
That's why we've had to continue to use the custom versions in the ceph rocksdb repo. We discussed dropping it at the perf meeting, but I think we still want to leave the fix in place for now. Can you make a new branch with that commit applied based on the newest version and we can try switching to that?
Mark
|
Looks like we still have the baggage of history, can't give up ceph/rocksdb |
@markhpc https://github.com/yanghonggang/rocksdb/tree/7.9.2.fb this is the new branch with ceph/rocksdb@6a2bb94 applied. But I don't know how to make a PR to create a new branch in ceph/rocksdb. |
|
|
Unable to push 7.9.2.fb branch to ceph/rocksdb
|
|
Another way is to sync fork directly in ceph/rocksdb
|
|
@jianwei1216 Let me see if I can find out who controls that repo and what the policy is |
|
@yanghonggang out of curiosity, does it work to fork the ceph rocksdb branch, make your own branch with the update from that one, and issue a pull request rather than trying to push a branch on the ceph rocksdb fork directly? |
|
|
In order to create a pull request, I have to specify a dest branch. But now all the existing ceph/rocksdb branches are not suitable. The ceph/rocksdb owner(or someone with privilege) can create a 7.9.2.fb branch, then I can create a pull request with ceph/rocksdb@6a2bb94 applied. To create a 7.9.2.fb branch: |
|
@yanghonggang ok, let me see what I can do. Sorry for the slow responses, but I do really want to get this in for Reef (which now is delays for 3 months, so we have a little more breathing room). |
|
@rzarzynski mentioned he can take a look as well as he has updated RocksDB via this method in the past as well. |
|
I think we should:
WIP. |
|
Just pushed https://github.com/ceph/rocksdb/tree/ceph-reef-v7.8.fb. |
@rzarzynski is it possible to use the current upstream release, as this PR is quite old - the title just wasn't updated?: https://github.com/facebook/rocksdb/releases 7.9.2 is the current upstream release. Thank you for doing this! |
|
I took the previous one from the submodule update of the current commit (f615ccd) in this PR. Pushed https://github.com/ceph/rocksdb/tree/ceph-reef-v7.9.2. |
- RocksDB community has confirmed the bad performance when compacting in 6.x and fixed in 7.5.3 * facebook/rocksdb#9423 * apache/kvrocks#1056 - new features(e.g., wal compression) Signed-off-by: Yang Honggang <yanghonggang_yewu@cmss.chinamobile.com>
f615ccd to
c743ed2
Compare
|
@rzarzynski thank you |
|
ping @markhpc |
rzarzynski
left a comment
There was a problem hiding this comment.
Personally I'm a bit nervous about merging this just before the freeze. However, I see the benefits, so let's at least try to get it in.
Adding needs-qa to allow testing to start sooner, without waiting for further approvals.
|
@rzarzynski I hear you. It's always scary doing these, but I think we've got to at least try given some of the fixes/improvements. |
|
Hi @yanghonggang, we tested your PR in teuthology and found some failures that look related: /a/lflores-2023-02-20_21:22:20-rados-wip-yuri-testing-2023-02-16-0839-distro-default-smithi/7181478 These all show the same valgrind memory leak error: Take a look and see what you think. See http://sprunge.us/QSf8ya for the full valgrind leak. @markhpc FYI |
|
Radek and I discussed this, I think we need to add a valgrind supression that looks something like: inslde https://github.com/ceph/ceph/blob/main/qa/valgrind.supp The gist of it is that this appears to only be a one time config parsing leak and a very small (8 byte) one at that. They may have changed something such that our previous supressions caught it but now they do not. |
|
@markhpc @rzarzynski I reran the same tests that failed without this PR, and they passed. Just wanted to confirm with you guys that this PR is solely responsible. |
This suppression is needed since upgrading to v7.9.2. See PR ceph#49006. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
This suppression is needed since upgrading to v7.9.2. See PR ceph#49006. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
```
// Get an CacheItemHelper pointer for value type T and role R.
template <typename T, CacheEntryRole R>
Cache::CacheItemHelper* GetCacheItemHelperForRole() {
static Cache::CacheItemHelper cache_helper(
BlocklikeTraits<T>::SizeCallback, BlocklikeTraits<T>::SaveToCallback,
GetCacheEntryDeleterForRole<T, R>());
return &cache_helper;
}
```
Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
339bb6c to
0de4d99
Compare
```
<frame>
<ip>0x1712F1E</ip>
<obj>/home/rzarzynski/ceph3/build/bin/ceph-mon</obj>
<fn>rocksdb::BytewiseComparator()</fn>
<dir>/home/rzarzynski/ceph3/src/rocksdb/util</dir>
<file>comparator.cc</file>
<line>304</line>
</frame>
<frame>
<ip>0x162361C</ip>
<obj>/home/rzarzynski/ceph3/build/bin/ceph-mon</obj>
<fn>rocksdb::ColumnFamilyOptions::ColumnFamilyOptions()</fn>
<dir>/home/rzarzynski/ceph3/src/rocksdb/options</dir>
<file>options.cc</file>
<line>123</line>
</frame>
<frame>
<ip>0x12CA21E</ip>
<obj>/home/rzarzynski/ceph3/build/bin/ceph-mon</obj>
<fn>Options</fn>
<dir>/home/rzarzynski/ceph3/build/../src/rocksdb/include/rocksdb</dir>
<file>options.h</file>
<line>1409</line>
</frame>
<frame>
<ip>0x12CA21E</ip>
<obj>/home/rzarzynski/ceph3/build/bin/ceph-mon</obj>
<fn>RocksDBStore::init(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)</fn>
<dir>/home/rzarzynski/ceph3/build/../src/kv</dir>
<file>RocksDBStore.cc</file>
<line>400</line>
</frame>
```
Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
|
Rados suite review: http://pulpito.front.sepia.ceph.com/?branch=wip-yuri-testing-2023-02-16-0839 Some failures were related to #49006, but were fixed locally due to lab issues. Failures, unrelated:
Details:
|
This suppression is needed since upgrading to v7.9.2. See PR ceph#49006. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
Signed-off-by: Yang Honggang yanghonggang_yewu@cmss.chinamobile.com
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows