Refactor local object storage #188

dstepanov-yadro · 2023-03-30T09:35:33Z

dstepanov-yadro commented

2023-03-30 09:35:33 +00:00

Local object storage refactorings

dstepanov-yadro force-pushed refactoring/object-3610_los from 094f286d01 to 5b1b7742ec

2023-03-30 09:42:21 +00:00

Compare

dstepanov-yadro force-pushed refactoring/object-3610_los from 36bcdd7e49 to 6921dd7734

2023-03-30 10:44:07 +00:00

Compare

dstepanov-yadro force-pushed refactoring/object-3610_los from e647a494a9 to 3b083f4a54

2023-03-30 15:49:58 +00:00

Compare

dstepanov-yadro force-pushed refactoring/object-3610_los from 3b083f4a54 to 73f58ebce7

2023-03-31 07:12:42 +00:00

Compare

dstepanov-yadro changed title from ~~WIP: Refactor local object storage~~ to Refactor local object storage

2023-03-31 11:27:38 +00:00

dstepanov-yadro requested review from storage-core-committers 2023-03-31 11:27:44 +00:00

dstepanov-yadro requested review from storage-core-developers 2023-03-31 11:27:44 +00:00

fyrchik commented

2023-04-03 08:31:11 +00:00

Can we use more specific prefixes in commits? los is new, but metabase: , blobstor: and engine: are well-established.

Can we use more specific prefixes in commits? `los` is new, but `metabase: `, `blobstor: ` and `engine: ` are well-established.

dstepanov-yadro force-pushed refactoring/object-3610_los from 5ed6da3e3a to 939d217bba

2023-04-03 09:54:15 +00:00

Compare

dstepanov-yadro commented

2023-04-03 09:54:34 +00:00

Can we use more specific prefixes in commits? los is new, but metabase: , blobstor: and engine: are well-established.

fixed

> Can we use more specific prefixes in commits? `los` is new, but `metabase: `, `blobstor: ` and `engine: ` are well-established. fixed

fyrchik approved these changes 2023-04-03 13:02:26 +00:00

aarifullin reviewed 2023-04-03 13:50:09 +00:00

pkg/local_object_storage/engine/evacuate.go Outdated

					
				@ -117,2 +181,3 @@

						}

					var res EvacuateShardRes

						if prm.handler == nil {

aarifullin commented

2023-04-03 13:36:39 +00:00

You check prm.handler for nil within the for-loop but it seems it's nowhere set or changed. Why isn't this check placed before for-loop at once?

You check `prm.handler` for `nil` within the for-loop but it seems it's nowhere set or changed. Why isn't this check placed before for-loop at once?

dstepanov-yadro commented

2023-04-03 16:29:56 +00:00

see lines above:

		if e.tryEvacuateObject(addr, getRes.Object(), sh, res, shards, weights, shardsToEvacuate) {
			continue
		}

		if prm.handler == nil {
			// Do not check ignoreErrors flag here because
			// ignoring errors on put make this command kinda useless.
			return fmt.Errorf("%w: %s", errPutShard, toEvacuate[i])
		}

It's possible that this if-statement will never checked.

see lines above: ``` if e.tryEvacuateObject(addr, getRes.Object(), sh, res, shards, weights, shardsToEvacuate) { continue } if prm.handler == nil { // Do not check ignoreErrors flag here because // ignoring errors on put make this command kinda useless. return fmt.Errorf("%w: %s", errPutShard, toEvacuate[i]) } ``` It's possible that this if-statement will never checked.

👍 1

pkg/local_object_storage/metabase/inhume.go Outdated

					
				@ -266,0 +225,4 @@

				//  2. zeroValue if Inhume was called with a GC mark

				func (db *DB) getInhumeTargetBucketAndValue(garbageBKT, graveyardBKT *bbolt.Bucket, prm *InhumePrm) (*bbolt.Bucket, []byte, error) {

					var (

						targetBucket *bbolt.Bucket

aarifullin commented

2023-04-03 13:23:44 +00:00

[Optionally] How about to make named return params?

(targetBucket *bbolt.Bucket, value []byte, err error)

From my point of view, it is better to use named parameters if their number is more than 2

[Optionally] How about to make **named** return params? ``` (targetBucket *bbolt.Bucket, value []byte, err error) ``` From my point of view, it is better to use named parameters if their number is more than 2

dstepanov-yadro commented

2023-04-03 16:34:21 +00:00

fixed