[#561] writecache: Make badger keys counting async #613
No reviewers
TrueCloudLab/storage-core-developers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#613
Loading…
Reference in a new issue
No description provided.
Delete branch "aarifullin/frostfs-node:feature/561-init_count"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Signed-off-by: Airat Arifullin a.arifullin@yadro.com
[#561] writecache: Make badger keys counting asyncto WIP: [#561] writecache: Make badger keys counting async9ab9c29769
tod858677aff
d858677aff
to5d76695294
WIP: [#561] writecache: Make badger keys counting asyncto [#561] writecache: Make badger keys counting async5d76695294
tod16417386e
@ -293,2 +293,4 @@
ShardCouldNotMarkObjectToDeleteInMetabase = "could not mark object to delete in metabase" // Debug in ../node/pkg/local_object_storage/shard/inhume.go
WritecacheBadgerInitExperimental = "initializing badger-backed experimental writecache"
WritecacheBadgerKeysCountByIteration = "failed to count keys from badger by iteration"
WritecacheBadgerKeysCountFlush = "failed to flush key_count to badger"
Do not use variable names from the code in the log messages. Otherwise, only the developer will be able to understand the log.
Fixed, check this out plz
Don't see.
@ -128,0 +139,4 @@
v := make([]byte, 8)
cDB, _ := c.objCounters.DB()
binary.LittleEndian.PutUint64(v, cDB)
if err := c.putKeyValue(k[:], v); err != nil {
If writecache is full, then this fails. I don't think it is correct behaviour.
You're right, but writing the
keys_count
down to badger is rather advisory than mandatory.This advisory writing makes the next
Open
invocation faster.So, do you have any suggestions how to process this case?
I think key_count could be just saved to DB, without full-check.
@ -83,0 +89,4 @@
return writecache.ErrOutOfSpace
}
wb := c.db.NewWriteBatch()
Since this method is used for flush, batch looks redundant.
To be honest, this definition has been taken from
above
@ale64bit
Can you tell, please, is the batch really redundant here?
There's not much point in having a
WriteBatch
for a single update.Update
can be used instead.This was introduced by me but it doesn't really match what bbolt does internally via the
Batch
call. Feel free to correct it.@ -11,2 +15,4 @@
var ErrInitializing = errors.New("db counter has been not initialized yet")
func (c *cache) estimateCacheSize() uint64 {
onDiskSize, _ := c.db.EstimateSize(nil)
The file size on disk is still used here, but bbolt uses the number of objects. But if we use a counter of objects, it is not clear how asynchronous counting will help us.
You're right, we don't need to use
DB()
if we want roughly estimate size of db data.But we cannot use
EstimateSize
if need the exact number of keys in DB:So, we won't be able to count the exact number using
EstimateSize
and the rough approximation for keys number may mislead if we use it for metrics or other purposescc @fyrchik @ale64bit
But such rough approximation with size rounding is used for bbolt.
For example, imagine that you are a user and you see that writecache is not being used. You open the metrics and see that the limit has not been reached. Or vice versa.
Probably, we talk about different things. Let me clarify:
DB()
gives the exact number of keys currently saved in badger.We cannot use
EstimateSize()
to calculate the number of keys:keys = EstimateSize() / EstimateKeySize()
- this is incorrect@ -55,0 +75,4 @@
if err = Delete(c.db, k[:]); err != nil {
c.log.Error(logs.WritecacheBadgerKeysCountDelete, zap.Error(err))
}
return
Here the function returns, but metrics and objCounter is not updated.
Right
But is it correct? Please explain, I probably don't understand the logic.
"Right" - I meant "you are right, I forgot to calculate metrics and objCounter" :)
I fixed this - I introduced
deleteKey
method that deletes key that is raw-represented. It makes trace spanning and calculate metricsd16417386e
toee27bae294
@ -71,0 +73,4 @@
// deleteKey removes object from write-cache by raw-represented key.
//
// Returns an error of type apistatus.ObjectNotFound if object is missing in write-cache.
func (c *cache) deleteKey(ctx context.Context, key []byte) error {
This is duplicated from
Delete
. I am wondering how this definition can be generilized@ -82,1 +82,4 @@
}
// putKeyValue persists raw-represent key and value to the write-cache
// database and pushes the to the flush workers queue.
pushes the to the flush -> pushes them to the flush?
ee27bae294
to75ea68b246
75ea68b246
to8302cf8f3c
8302cf8f3c
tocc71a44eba
cc71a44eba
toa7de302632
a7de302632
tobf45a3da36
bf45a3da36
toa1fa15c535
a1fa15c535
to07383f823a
07383f823a
tof45ebc0199
f45ebc0199
toea7af01d6f
ea7af01d6f
toef2e8e1aea
ef2e8e1aea
toad8c095391
@ -103,2 +127,4 @@
// Close closes db connection and stops services. Executes ObjectCounters.FlushAndClose op.
func (c *cache) Close() error {
c.initCounterWG.Wait()
Do we have a way to cancel this process?
I have introduced the wait method that is cancelled by timeout
So not we wait for some timeout, but it seems we still don't cancel anything.
Key counting process is completely auxiliary and if we have it, this means we didn't have key count on startup, thus we do not lose anything by canceling it.
It seems wrong to block by any time here.
Okay. It seems you have meant
I was confused but for now I got your idea. I will make cancellation for counting
@fyrchik
Please, check out how I have implemented soft cancellation
And still we need to have
c.initCounterWG.Wait()
to correctly finish gorotine. You can check explanation forcancelInitCountBySetModeAndWait
@ -96,0 +108,4 @@
return value, metaerr.Wrap(err)
}
func (c *cache) getRawKeyInternal(key []byte) ([]byte, error) {
Why
getRawKey
andgetRawKeyInternal
are two different functions?@ -23,3 +41,4 @@
}
func (x *counters) IncDB() {
x.txWG.Wait()
So new operations will hang until counters are finished?
Until db transaction starts :)
Removed this waitGroup and replaced it with
isMutable
checkP.S. I am going to resolve the conversation to make the comments readable
@ -32,3 +52,3 @@
func (x *counters) DB() uint64 {
return x.cDB.Load()
_, ok := <-x.keysCountDone
Again, probably not a good idea: it is easy to miss that "give me the object number" can be blocked by some uncancellable process iterated through the whole database.
Hence
DB
invocation cannot be blocked (I've removedkeysCountDone
) but it returns 0 if the value has not been initilized yet. I think returning 0 is the best option and better than returning errorErrNotInitYet
P.S. I am going to resolve the conversation to make the comments readable
@ -55,0 +74,4 @@
if !c.options.recountKeys {
k := keyCountPrefix()
cDB, err := c.getRawKey(context.Background(), k[:])
Do we want auxiliary info receival affecting datapath metrics?
Sorry, what is the focus of this comment at this line?
You don't like the idea that metrics are counter within
getRawKey
?I mean that
getRawKey
contains metrics and traces, but what we get here is not user data we store.I can agree about metrics but I would like to to offer a compromise option and make
getRawKey
tracewritecache.getRawKey
event but not calculate user metrics. WDYT?Seems ok, but why bother with traces? We have
context.Background()
so it will be just a small trace unrelated to anything.Alright. Initially I misunderstood how
StartSpanFromContext
and now I got the point that it does not make sense to pass context untilinitCounter <- Open ....
recieve context. So, the refactoring is needed at first. I will create an issue andgetRawKey
anddeleteRawKey
no longer havectx
parameter (for a while)P.S. I am going to resolve the conversation to make the comments readable
P.S. I am going to resolve the conversation to make the comments readable
@ -42,6 +42,13 @@ func addr2key(addr oid.Address) internalKey {
return key
}
func keyCountPrefix() internalKey {
InternalKey is a fixed-length key denoting an address (so iteration is possibly affected). It is a valid address, can we explicitly make it different size and type?
Good point. I'll fix it
I don't see it being changed.
Sorry, I forgot about the comment. It seems you accidently resolved conv
ad8c095391
to9d8583b946
9d8583b946
toaa87a3ea63
aa87a3ea63
to1f1302fd52
1f1302fd52
to7b66d52ded
@ -21,0 +33,4 @@
init bool
// initMtx is mutex for init flag.
initMtx sync.RWMutex
We have a mutex for a single primitive variable, why not just use atomics? (a question, not a suggestion)
Mutex could give stronger guarante, but I dont see them being taken for the whole operation duration (currently we have a race condition, but possibly do not exploit it)
7b66d52ded
to621170148e
621170148e
to7923b2fe0e
@ -24,0 +24,4 @@
const metaInformationKeyLength = 20
// metaKey is a type that defines a key for helpful purposes.
type metaKey [metaInformationKeyLength]byte
@fyrchik Here is different type and length for
keys_count
key. It seems it doesn't break anything (I hope so)Now why 20? Isn't it just
len("keys_count") == 10
?I mean we can have global
var keyKey = []byte("keys_count")
without any functions (name can be changed)@ -29,3 +32,4 @@
store
}
var _ writecache.Cache = (*cache)(nil)
How is this related to the PR?
Removed
@ -104,1 +126,4 @@
func (c *cache) Close() error {
c.cancelInitCountByCloseAndWait()
c.flushKeysCount()
Is it correct to flush keys if we canceled counter iteration?
If you look at
flushKeysCount()
definition then you can see that if objCounters is not ready to be flushed then nothing will happen. I can move these checks out offlushKeysCount
@ -96,0 +100,4 @@
func (c *cache) getRawKey(key []byte) ([]byte, error) {
value, err := Get(c.db, key)
if err != nil {
return nil, metaerr.Wrap(new(apistatus.ObjectNotFound))
Is it always
NotFound
?Sorry, you're right. The raw key has nothing common with
ObjectNotFound
@ -55,0 +175,4 @@
// Recount keys if "key_count" has not been found
if err := c.db.View(func(tx *badger.Txn) error {
c.objCounters.cDB.setAsMutable()
I think it still may be invalid: snapshot for
View
is fixed before function executes and we "set as mutable" inside a function, so some objects may possibly arrived and will not be in counters. Am I right?T1
moment. We haveN
objects atT1
T2
moment. It'll try to increase object counterobjCounters
and we haveM
objects while counting is going on the snapshot[T1, Tk]
, we can inc/decobjCounters
. Iterators are valid withinView
N + M
- good resultSo, is something wrong?
7923b2fe0e
toa537d0a62f
Blocked by #688
Badger is dropped in #887
Pull request closed