ARSN-648: replace archived level/level-sublevel with classic-level - #2713
francoisferrand wants to merge 2 commits into
Conversation
Hello francoisferrand,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/8.6 #2713 +/- ##
===================================================
+ Coverage 74.80% 74.87% +0.06%
===================================================
Files 229 230 +1
Lines 18686 18729 +43
Branches 3909 3916 +7
===================================================
+ Hits 13978 14023 +45
+ Misses 4703 4701 -2
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
611139e to
70e2b1a
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
level@5 and level-sublevel are archived and drag in an old levelup / abstract-leveldown / leveldown chain, along with audit debt (bl memory exposure, semver ReDoS). classic-level is the maintained successor and embeds the same leveldown storage engine, so existing databases keep working as-is. The API moved on though: abstract-level is promise-first, dropped the NotFoundError that callers detect through err.notFound, and nests sublevels as '!a!!b!key' where level-sublevel flattened the whole path into '!a#b!key'. The file backend moves onto that API directly, keeping only what our own contracts need: the '#' flattened sublevel paths that keep databases written by level-sublevel readable, the sublevel lookup cache, batch operations that carry a sublevel path across RPC rather than a handle, and the iterator to Readable bridge the RPC layer needs to pipe listings back. IndexTransaction keeps its callback based batch: it commits to the handle its caller provides, which over RPC is the metadata client. Issue: ARSN-648
70e2b1a to
8f15a5a
Compare
delthas
left a comment
There was a problem hiding this comment.
LGTM but would be easier to review with format changes moved into their own commit
| return undefined; | ||
| } | ||
| for (let i = 0; i < nbKeys; ++i) { | ||
| client.withRequestLogger(reqLogger) |
There was a problem hiding this comment.
Hm, I feel this commit mixes preexisting prettier file changes with actual changes. Would it be possible to move preexisting prettify into its own first commit?
| // level-sublevel stored a nested path as a single '!a#b!key' section, where | ||
| // abstract-level nests them as '!a!!b!key'. Flattening the path into one | ||
| // sublevel name reproduces the historical layout, so databases written by | ||
| // previous versions stay readable. |
There was a problem hiding this comment.
This is hard to understand, feels like its written from the pov of doing a migration (so having the context of level-sublevel) instead of focusing on what it is doing
There was a problem hiding this comment.
Ok I guess its easier to understand when knowing about leveldb
SylvainSenechal
left a comment
There was a problem hiding this comment.
One comment from claude might be relevant.
I think it could be nice to have a review from hopo team too
The default lookup nested sublevels per path component, which used to match the level-sublevel layout but now yields '!a!!b!key' on classic-level, and it dropped the root encodings on the way. Rather than keep a fallback that silently writes keys nobody can read back, callers now have to pass the lookup they want. Issue: ARSN-648 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
level@~5.0.1andlevel-sublevel@~6.6.5are archived, and drag in an equally oldlevelup/abstract-leveldown/bytewisestack carrying three advisories. Moving the file metadata backend onto maintainedclassic-leveltakes that to zero, and drops 47 transitive deps.The risky part: on-disk compatibility
Deployed stores have to keep working, so the key layout must stay byte-identical.
level-sublevel's encoding isn't what nativeabstract-levelsublevels produce by nesting, so this reproduces it explicitly rather than relying on the new default.Verified against a real legacy store: wrote one with
level@5+level-sublevel@6.6.5, then opened it with this code — root, bucket and nested recordLog sublevels, stream ordering, and new writes landing in the same namespace as the legacy ones.tests/unit/storage/metadata/file/levelUtils.spec.jspins the raw key bytes so this can't drift silently.Scope
MetadataFileServer,RecordLogandlevel-netmove onto theabstract-levelAPI directly, with no compatibility facade left behind. What remains are helpers for our own contracts rather than the library's: the#flattened sublevel paths, the sublevel lookup cache, batch operations carrying a sublevel path across RPC rather than a handle, and the iterator toReadablebridge the RPC layer needs to pipe listings back.lib/db.tsis left alone —IndexTransaction.commit()takes a caller-supplied handle, so changing it would be a public API break outside this ticket.Also fixes a shadowed
errinmetadataAPI.batchthat was silently swallowingcreateLogRecordOpsfailures.Validation
Worth flagging: this backend's own coverage is thin (
MetadataFileServer.initMetadataServiceat 6.5% function coverage,BucketFileInterfaceat 5%). Pre-existing and not made worse here, but it does mean the real net under this change is CloudServer'sS3BACKEND=filefunctional suite rather than Arsenal's. Probably deserves a follow-up ticket.Issue: ARSN-648