Badgerstore #833

Closed
dstepanov-yadro wants to merge 7 commits from dstepanov-yadro/frostfs-node:feat/badgerstore into master

Another small object store implemenation.

References #555

Another small object store implemenation. References #555
requested reviews from storage-core-committers, storage-core-developers 2023-11-29 17:05:19 +00:00
dstepanov-yadro reviewed 2023-11-29 17:08:05 +00:00
@ -0,0 +40,4 @@
//
// Badger uses Dynamic Level Sizes like RocksDB.
// See https://github.com/facebook/rocksdb/blob/v3.11/include/rocksdb/options.h#L366 for explanation.
func defaultCfg() *cfg {
Author
Member

badger config

badger config
dstepanov-yadro force-pushed feat/badgerstore from f47b48d5b2 to 207820df5e 2023-11-30 13:53:40 +00:00 Compare
aarifullin reviewed 2023-12-01 07:55:02 +00:00
@ -0,0 +6,4 @@
)
func key(add oid.Address) []byte {
res := make([]byte, 64)
Member

Could you move 64 and 32 to constants, please?

Could you move `64` and `32` to constants, please?
Author
Member

done

done
aarifullin marked this conversation as resolved
dstepanov-yadro force-pushed feat/badgerstore from 207820df5e to 12b80b859d 2023-12-05 08:54:21 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from 12b80b859d to 11f5af27cd 2023-12-07 13:11:46 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from 11f5af27cd to 3e1e0393d7 2023-12-08 12:17:56 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from 3e1e0393d7 to d7c723d310 2023-12-08 12:22:36 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from d7c723d310 to d6ead4d10e 2023-12-08 15:05:07 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from d6ead4d10e to 7ea306d86c 2023-12-08 15:07:33 +00:00 Compare
aarifullin reviewed 2023-12-14 11:24:57 +00:00
@ -0,0 +62,4 @@
}
batch = append(batch, kv)
last = kv.key
if len(batch) == opts.PrefetchSize-1 {
Member

My point is absolutely optional but this check looks like it can be placed to

for it.Seek(last); it.Valid() && len(batch) < opts.PrefetchSize-1; it.Next() {
 /**/
}

:)

My point is absolutely optional but this check looks like it can be placed to ```go for it.Seek(last); it.Valid() && len(batch) < opts.PrefetchSize-1; it.Next() { /**/ } ``` :)
aarifullin marked this conversation as resolved
aarifullin approved these changes 2023-12-14 12:06:32 +00:00
aarifullin left a comment
Member

Brilliant

Brilliant
dstepanov-yadro force-pushed feat/badgerstore from 7ea306d86c to 5b5dc5a50d 2023-12-14 13:38:39 +00:00 Compare
aarifullin reviewed 2023-12-14 15:04:22 +00:00
@ -0,0 +62,4 @@
}
batch = append(batch, kv)
last = kv.key
if len(batch) == opts.PrefetchSize-1 {
Member

Hm. Just curious about if this check is really necessary here. The iterator it is initialized with the option opts.PrefetchSize. Isn't it invalidated on itself when it fetches opts.PrefetchSize items?

Docs: iterating-over-keys

Hm. Just curious about if this check is really necessary here. The iterator `it` is initialized with the option `opts.PrefetchSize`. Isn't it invalidated on itself when it fetches `opts.PrefetchSize` items? [Docs: iterating-over-keys ](https://dgraph.io/docs/badger/get-started/#iterating-over-keys)
Author
Member

This is done so that there are no long transactions. So storage reads one batch within transaction, returns results to invoker, reads another batch within transaction, etc.

This is done so that there are no long transactions. So storage reads one batch within transaction, returns results to invoker, reads another batch within transaction, etc.
aarifullin marked this conversation as resolved
acid-ant approved these changes 2023-12-15 09:55:40 +00:00
@ -0,0 +47,4 @@
return MemTablesCountDefault
}
// CompactorsCount returns `mem_tables_count` value or CompactorsCountDefault.
Member

mem_tables_count -> compactors_count

mem_tables_count -> compactors_count
Author
Member

fixed

fixed
acid-ant marked this conversation as resolved
@ -0,0 +96,4 @@
to := from + prm.Range.GetLength()
payload := obj.Payload()
if pLen := uint64(len(payload)); to < from || pLen < from || pLen < to {
Member

How about to check this to < from before get?

How about to check this `to < from` before `get`?
Owner

I would leave this code as is -- it is encountered in multiple places (basically, every GetRange) and it is not straightforward to do it right (wrong boundary checks can easily lead to panic or OOM), so now we can just copy it once tested.

I would leave this code as is -- it is encountered in multiple places (basically, every GetRange) and it is not straightforward to do it right (wrong boundary checks can easily lead to panic or OOM), so now we can just copy it once tested.
acid-ant marked this conversation as resolved
dstepanov-yadro force-pushed feat/badgerstore from 5b5dc5a50d to 1b6a06e603 2023-12-20 15:13:24 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from 1b6a06e603 to bd2be1f64e 2023-12-27 11:56:05 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from b2b28b3697 to 82bf0cd2c8 2024-01-09 07:02:38 +00:00 Compare
aarifullin approved these changes 2024-01-10 11:05:54 +00:00
acid-ant approved these changes 2024-01-10 11:20:39 +00:00
dstepanov-yadro force-pushed feat/badgerstore from 82bf0cd2c8 to 5399a852ed 2024-01-10 11:45:23 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from 5399a852ed to c02315fc80 2024-01-15 14:09:21 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from c02315fc80 to 1d4732c2a8 2024-01-18 18:51:40 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from 1d4732c2a8 to ffc7b32197 2024-01-26 10:37:19 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from ffc7b32197 to 6ebbada07c 2024-02-13 12:29:15 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from 6ebbada07c to 824251f650 2024-02-14 14:33:12 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from 20195677a5 to 9f7b114d4f 2024-02-15 10:09:09 +00:00 Compare
requested review from fyrchik 2024-02-21 09:21:08 +00:00
aarifullin approved these changes 2024-02-22 11:15:51 +00:00
Owner

I will wait until v0.38, then merge

I will wait until v0.38, then merge
dstepanov-yadro force-pushed feat/badgerstore from 9caf0f9a6d to f5e7a7cffb 2024-04-09 13:54:58 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from f5e7a7cffb to fba92a3d61 2024-04-09 14:01:05 +00:00 Compare
dstepanov-yadro force-pushed feat/badgerstore from fba92a3d61 to ec81c1bcfd 2024-04-09 14:09:58 +00:00 Compare
dstepanov-yadro closed this pull request 2024-06-24 13:49:35 +00:00
Some checks failed
DCO action / DCO (pull_request) Successful in 1m57s
Required
Details
Vulncheck / Vulncheck (pull_request) Successful in 4m45s
Required
Details
Build / Build Components (1.21) (pull_request) Successful in 6m18s
Required
Details
Build / Build Components (1.20) (pull_request) Successful in 6m32s
Required
Details
Tests and linters / Staticcheck (pull_request) Successful in 7m17s
Required
Details
Tests and linters / Lint (pull_request) Successful in 7m54s
Required
Details
Tests and linters / Tests (1.21) (pull_request) Successful in 15m21s
Required
Details
Tests and linters / Tests with -race (pull_request) Successful in 15m27s
Required
Details
Tests and linters / Tests (1.20) (pull_request) Successful in 15m54s
Required
Details
Tests and linters / gopls check (pull_request) Failing after 1m59s
Required
Details

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#833
No description provided.