search

zaidoon1 / rust-rocksdb

rust wrapper for rocksdb
Apache License 2.0
12 stars 3 forks source link

Allow setting logging callback. #54

Closed jevolk closed 1 month ago

jevolk commented 7 months ago

This will need https://github.com/facebook/rocksdb/pull/12537 Screenshot from 2024-04-15 00-24-16

jevolk commented 6 months ago

std::ptr::from_ref (1.76) slightly exceeds the MSRV (1.75). Would you like me to downgrade that or are you interested in bumping?

zaidoon1 commented 5 months ago

sorry, missed the notification for this for some reason, initially I wanted to stay with the latest, however, it looks like this is not recommended as it may cause issues for users of this crate. I'll likely adopt a similar policy to tokio. So let's not try to upgrade the MSRV for now.

girlbossceo commented 5 months ago

Do you know any specific users you'd like to reach out to for permission to bump the MSRV in your fork of rust-rocksdb? conduwuit would really like this feature, our MSRV is already the latest.

zaidoon1 commented 5 months ago

would really like this feature

sorry, maybe i'm misunderstanding, the std::ptr::from_ref requires the new version, but you don't care about this part right? You can do it a different way (based on the comment Would you like me to downgrade that) and still get this pr/feature merged

girlbossceo commented 5 months ago

Yes just using an as conversion is fine, but I'm just wondering if an exception for a MSRV raise could be done here. If not, that's fine and you're right this feature will still land either way.

zaidoon1 commented 5 months ago

we can bump the MSRV one more time before we restrict it to the 6 month rolling releases.

girlbossceo commented 2 months ago

This has landed in RocksDB v9.5.2: https://github.com/facebook/rocksdb/commit/8bf1f6f87f31a611cd12d9fcdebffcb51ea476fe

image

Wasn't noted in their release notes though, but the commit is in the tag

zaidoon1 commented 2 months ago

@girlbossceo SGTM, if @jevolk can fix up the merge conflicts/get the pr in a mergeable state, I can merge it.

jevolk commented 2 months ago

I have removed std::ptr::from_ref so changing the MSRV is no longer necessary. Hopefully this means the patch can also find its way upstream eventually rather than anyone duplicating the effort.

Once CI passes i'm just going to run this again in vivo in my application before marking it ready.

Thanks for your patience 🙏🏻