TrueCloudLab/frostfs-node

Fork 27

Count items in the writecache faster #561

New issue

Closed

opened 2023-08-03 08:39:11 +00:00 by fyrchik · 7 comments

fyrchik commented

2023-08-03 08:39:11 +00:00

Owner

After #462 (comment) we return to the iteration scheme for counting the amount of items in bucket. We aim at to start fast, so here is what we could do:

Restore async writecache initialization (return ErrInitializing until iteration has finished)
On graceful shutdown store the number of items by some key, read and remove it on startup.

After https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/462#issuecomment-17578 we return to the `iteration` scheme for counting the amount of items in bucket. We aim at to start fast, so here is what we could do: 1. Restore async writecache initialization (return `ErrInitializing` until iteration has finished) 2. On _graceful_ shutdown store the number of items by some key, read and remove it on startup.

fyrchik added the

frostfs-node

triage

blocked

labels 2023-08-03 08:39:11 +00:00

fyrchik referenced this issue

2023-08-03 08:39:21 +00:00

[#421] Try using badger for the write-cache #462

aarifullin self-assigned this 2023-08-03 10:55:28 +00:00

aarifullin commented

2023-08-04 09:18:58 +00:00

Member

I think the first option is better. Let's look at the defs:

type counters struct {
	cDB atomic.Uint64
}

func (x *counters) IncDB() {
	x.cDB.Add(1)
}

func (x *counters) DecDB() {
	x.cDB.Add(math.MaxUint64)
}

func (x *counters) DB() uint64 {
	return x.cDB.Load()
}

So, after startup:

IncDB() and DecDB() can finely work, if x.cDB is not set yet(it's kind of working with blackbox)
When we really need to retrieve value, we invoke DB()

func (x *counters) DB() uint64 {
	dbCount, ok := x.dbCountCh // it is kind of future-promise concept
    if !ok {
       return x.cDB.Load()
    }
    x.cDB.Add(dbCount)
    return x.cDB.Load()
}

And run initCounters() asynchronously

func (c *cache) initCounters() error { 
   ...
   c.objCounters.dbCountCh <- inDB
   close(c.objCounters.dbCountCh)
}

@fyrchik @ale64bit WDYT?

I think the first option is better. Let's look at the defs: ```golang type counters struct { cDB atomic.Uint64 } func (x *counters) IncDB() { x.cDB.Add(1) } func (x *counters) DecDB() { x.cDB.Add(math.MaxUint64) } func (x *counters) DB() uint64 { return x.cDB.Load() } ``` So, after startup: 1. `IncDB()` and `DecDB()` can finely work, if `x.cDB` is not set yet(_it's kind of working with blackbox_) 2. When we _really_ need to retrieve value, we invoke `DB()` ```golang func (x *counters) DB() uint64 { dbCount, ok := x.dbCountCh // it is kind of future-promise concept if !ok { return x.cDB.Load() } x.cDB.Add(dbCount) return x.cDB.Load() } ``` And run `initCounters()` asynchronously ```golang func (c *cache) initCounters() error { ... c.objCounters.dbCountCh <- inDB close(c.objCounters.dbCountCh) } ``` @fyrchik @ale64bit WDYT?

fyrchik commented

2023-08-04 09:22:28 +00:00

Author

Owner

To me (1) and (2) are not mutually exclusive.
Speaking of (1) I think we better fail requests prematurely and not hang for an undefined amount of time.

To me (1) and (2) are not mutually exclusive. Speaking of (1) I think we better fail requests prematurely and not hang for an undefined amount of time.

aarifullin commented

2023-08-04 09:26:44 +00:00

Member

To me (1) and (2) are not mutually exclusive.

Hm. Am I right that if we cannot find the key from (2), then we go to (1)?

Speaking of (1) I think we better fail requests prematurely and not hang for an undefined amount of time.

Yes, it should be like that. But my suggestion is still compatible with (1)

> To me (1) and (2) are not mutually exclusive. Hm. Am I right that if we cannot find the key from (2), then we go to (1)? > Speaking of (1) I think we better fail requests prematurely and not hang for an undefined amount of time. Yes, it should be like that. But my suggestion is still compatible with (1)

ale64bit commented

2023-08-04 12:54:24 +00:00

Member

@fyrchik do we need to be able to count the keys? can't we get away without it? (e.g. by using the disk space instead, which is more commonly provided)

aarifullin commented

2023-08-04 14:21:11 +00:00

Member

I suppose this requires to redesign the way for cache initialization

If we take a look at this Open

// Open opens and initializes database. Reads object counters from the ObjectCounters instance.
func (c *cache) Open(readOnly bool) error {
	err := c.openStore(readOnly)
	if err != nil {
		return metaerr.Wrap(err)
	}

	// Opening after Close is done during maintenance mode,
	// thus we need to create a channel here.
	c.closeCh = make(chan struct{})

	return metaerr.Wrap(c.initCounters())
}

For now we take Open for failed one, if we cannot read/count the amount of objects within db. But if we make initCounters async call, then we are obliged to ignore potential error witihn Open because it may occur only later.
Therefore, we can take Open for successful invocation (-> succesful initialization), however initCounters may fail later.
Anyway, it is not the big deal to take initCounter away from Open and make the call standalone.

But if we somehow get "promised" error from initCounters in the case it failed, then we cannot stop the work and we always return error from DB() call and metrics will be always incorrect or we need to ignore metric calculation - what do we do in this case?

I suppose this requires to redesign the way for cache initialization If we take a look at this `Open` ```golang // Open opens and initializes database. Reads object counters from the ObjectCounters instance. func (c *cache) Open(readOnly bool) error { err := c.openStore(readOnly) if err != nil { return metaerr.Wrap(err) } // Opening after Close is done during maintenance mode, // thus we need to create a channel here. c.closeCh = make(chan struct{}) return metaerr.Wrap(c.initCounters()) } ``` For now we take `Open` for failed one, if we cannot read/count the amount of objects within db. But if we make `initCounters` async call, then we are obliged to ignore potential error witihn `Open` because it may occur only later. Therefore, we can take `Open` for successful invocation (-> succesful initialization), however `initCounters` may fail later. Anyway, it is not the big deal to take `initCounter` away from `Open` and make the call standalone. But if we somehow get "promised" error from `initCounters` in the case it failed, then we cannot stop the work and we always return `error` from `DB()` call and metrics will be always incorrect or we need to ignore metric calculation - what do we do in this case?

fyrchik added the

badger

label 2023-08-09 10:51:10 +00:00

aarifullin removed the

blocked

label 2023-08-15 11:57:49 +00:00

aarifullin referenced this issue

2023-08-15 21:52:18 +00:00

[#561] writecache: Make badger keys counting async #613

fyrchik commented

2023-08-17 07:37:29 +00:00

Author

Owner

We can block cache from accepting requests until counters are initialized -- need to have some ballpark numbers here for the duration.
Or we may remove this metric alltogether (though it seems useful).

Having it for bolt and not for badger seems wrong.

We can block cache from accepting requests until counters are initialized -- need to have some ballpark numbers here for the duration. Or we may remove this metric alltogether (though it seems useful). Having it for bolt and not for badger seems wrong.

aarifullin was unassigned by fyrchik

2024-08-09 06:21:58 +00:00

fyrchik closed this issue

2024-08-09 06:22:12 +00:00

fyrchik commented

2024-08-09 06:22:21 +00:00

Author

Owner

Badger is no longer a thing.