Replace some rocksdb databases with lmdb #15810
Conversation
|
!ci-build-me |
|
!ci-toolchain-me |
Geometer1729
force-pushed
the
brian/more-lmdb
branch
from
7faf5fb
to
e96ed65
Compare
There was a problem hiding this comment.
Not approving since this is for now a draft PR. Keep up the good work @Geometer1729
| ; path = name | ||
| ; uuid = | ||
| name_to_uuid name | ||
| (* Can this be the same, if not what can I use instead of the path? *) |
There was a problem hiding this comment.
what can I use instead of the path
Path and something related to a timestamp ?
|
|
||
| let to_alist t = | ||
| Lmdb.Cursor.fold_left ~f:(fun xs k v -> List.cons (k, v) xs) [] t.lmdb | ||
| (* TODO this feels like a bad idea performance wise *) |
There was a problem hiding this comment.
It is a bad idea. However, for this PR, it is fair to keep the same functionalities.
A quick grep within the codebase shows it's mostly used to List.map over the resulting alist.
This functionality (map) should probably be offered by this module instead to avoid allocating (possibly) big intermediate structures.
Removing usage of to_alist looks like a benefitial follow-up task to me. If we actually need a sequence, we should probably implement a to_seq function instead.
src/lib/mina_ledger/lmdb_kvdb.ml
Outdated
| let get t ~key = | ||
| try Some (Lmdb.Map.get t.lmdb key) with Not_found_s _ -> None | ||
| (* | Not_found -> None *) | ||
| (* TODO does Not_found_s subsume Not_found ? if not how do I get the compiler to let me do this? *) |
There was a problem hiding this comment.
2 comments:
- Documentation seems to state that
getreturnsNot_foundnotNot_found_s. - The compiler cannot help here. Exceptions are extensible variants, that the compiler cannot check for exhaustiveness. There have been some effort here and there to analyze whether all exceptions were caught (see this discussion)
There was a problem hiding this comment.
Sorry my comment was not very clear, what I meant was how do I get the compiler not to complain about Not_found being deprecated
File "src/lib/mina_ledger/lmdb_kvdb.ml", line 61, characters 9-18:
> 61 | with Not_found (* LMDB raises Not_found when the key is absent *) -> ()
> ^^^^^^^^^
> Error (alert deprecated): Not_found
it seems to be [@alert "-deprecated"]
There was a problem hiding this comment.
Understood. This is the reason.
The solution here is to disable the alert for this function (and link to the API stating where the Not_found comes from).
This should work
let[@alert "-deprecated"] get ~key =
try Some (Lmdb.Map.get t.lmdb key) with Not_found -> None
Co-authored-by: Richard Bonichon <[email protected]>
Co-authored-by: Richard Bonichon <[email protected]>
Co-authored-by: Richard Bonichon <[email protected]>
|
@rbonichon Thanks so much for looking this over and for your suggestions |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
Explain your changes:
*
Explain how you tested your changes:
*
Checklist: