Simplify write-cache (SUPPORT) #353

fyrchik · 2023-05-15T14:53:11Z

fyrchik commented

2023-05-15 14:53:11 +00:00

Based on #314 minus counters logic.

fyrchik added 2 commits 2023-05-15 14:53:12 +00:00

[#314 ] wc: Simplify background workers naming ab65063d6d

Also, drop not used arg.

Signed-off-by: Pavel Karpy <p.karpy@yadro.com>

[#314 ] wc: Do not lose small objects on disk errors c3f5045842

Do return error if an object could not been stored on WC's disk.

Signed-off-by: Pavel Karpy <p.karpy@yadro.com>

fyrchik reviewed 2023-05-15 14:53:28 +00:00

pkg/local_object_storage/writecache/flush.go Outdated

					
				@ -71,3 +71,2 @@

						c.modeMtx.RLock()

						if c.readOnly() || !c.initialized.Load() {

						// TryRLock doc explicitly mentions that correct usages are rare.

fyrchik commented

2023-05-15 14:53:28 +00:00

I am open to suggestions here.

fyrchik commented

2023-05-16 12:14:02 +00:00

Removed this, make closing in 2 steps.

fyrchik force-pushed simplify-cache-support from e0094e9516 to 731e526ddd

2023-05-15 14:53:56 +00:00

Compare

fyrchik requested review from storage-core-committers 2023-05-15 14:55:34 +00:00

fyrchik requested review from storage-core-developers 2023-05-15 14:55:34 +00:00

JuliaKovshova approved these changes 2023-05-15 14:57:10 +00:00

ale64bit reviewed 2023-05-16 08:19:50 +00:00

pkg/local_object_storage/writecache/writecache.go Outdated

					
				@ -167,16 +156,17 @@ func (c *cache) Close() error {

						return err

					}

					fmt.Println(c.closeCh == nil)

ale64bit commented

2023-05-16 08:19:50 +00:00

are these supposed to be committed?

👍 1

fyrchik commented

2023-05-16 11:18:14 +00:00

fixed

ale64bit marked this conversation as resolved

ale64bit commented

2023-05-16 09:19:54 +00:00

Disregarding the usage of TryLock which is usually a smell (perhaps a Cond is better here), I find the code really difficult to read and reason about. For example, I would be afraid of changing anything :) I'm not sure if some extra code comments or assertions would help to understand which invariants are maintained throughout the flushing process.

Disregarding the usage of `TryLock` which is usually a smell (perhaps a `Cond` is better here), I find the code really difficult to read and reason about. For example, I would be afraid of changing anything :) I'm not sure if some extra code comments or assertions would help to understand which invariants are maintained throughout the flushing process.

👍 1

fyrchik commented

2023-05-16 10:58:39 +00:00

@ale64bit how exactly can you use Cond here?

@ale64bit how exactly can you use `Cond` here?

fyrchik force-pushed simplify-cache-support from 731e526ddd to 3711976dfc

2023-05-16 11:17:22 +00:00

Compare

acid-ant approved these changes 2023-05-16 11:22:25 +00:00

dstepanov-yadro approved these changes 2023-05-16 11:32:21 +00:00

fyrchik merged commit 3711976dfc into support/v0.36

2023-05-16 12:39:12 +00:00