policer: add check for repeated error logging #914

elebedeva · 2024-01-16T14:09:09Z

elebedeva commented

2024-01-16 14:09:09 +00:00

Relates #110

processObject() returns 3 types of errors:

container not found errors
could not get container error
placement vector building error

Every error will occur for all objects in container simultaneously, so we can log each error once and safely ignore the rest.

Signed-off-by: Ekaterina Lebedeva ekaterina.lebedeva@yadro.com

Relates #110 ```processObject()``` returns 3 types of errors: 1. container not found errors 2. could not get container error 3. placement vector building error Every error will occur for all objects in container simultaneously, so we can log each error once and safely ignore the rest. Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>

elebedeva requested review from storage-core-committers 2024-01-16 14:09:48 +00:00

elebedeva requested review from storage-core-developers 2024-01-16 14:09:51 +00:00

aarifullin reviewed 2024-01-16 14:41:25 +00:00

pkg/services/policer/process.go Outdated

					
				@ -71,2 +77,4 @@

					}

				}

				func skipLog(errs []string, err string) bool {

aarifullin commented

2024-01-16 14:41:25 +00:00

I am sure this method works fine in your case :) And my change request is absolutely optional. But it works until incoming err gets wrapped for some reason by another error. So, better to not compare them like strings, but make

cnrErrSkip := make(map[cid.ID][]error)

and then

import errors
/* ... */
func skipLog(errs []error, err err) bool {
   for _, e := range errs {
      if errors.Is(err, e) {
         return true
      }
   }
   return false
}

I am sure this method works fine in your case :) And my change request is absolutely optional. But it works until incoming `err` gets wrapped for some reason by another error. So, better to not compare them like strings, but make ```go cnrErrSkip := make(map[cid.ID][]error) ``` and then ```go import errors /* ... */ func skipLog(errs []error, err err) bool { for _, e := range errs { if errors.Is(err, e) { return true } } return false } ```

👍 1

fyrchik commented

2024-01-17 12:48:01 +00:00

Btw, another think to think about is that we can fail to connect to different nodes -- I would like to see this in logs.
But it is probably not too important -- if a node is isolated, lot's of other errors will occur.

Btw, another think to think about is that we can fail to connect to different nodes -- I would like to see this in logs. But it is probably not too important -- if a node is isolated, lot's of other errors will occur.

fyrchik reviewed 2024-01-16 16:13:41 +00:00

pkg/services/policer/process.go Outdated

					
				@ -53,10 +57,12 @@ func (p *Policer) shardPolicyWorker(ctx context.Context) {

									if p.objsInWork.add(addr.Address) {

										err := p.processObject(ctx, addr)

fyrchik commented

2024-01-16 16:13:25 +00:00

processObject can return errors not only from container build, but also because some nodes are unavailable.
Can you give a list of errors (not full, just some groups) which we will ignore here?
It seems everything is ok, the last error is

return fmt.Errorf("%s: %w", logs.PolicerCouldNotBuildPlacementVectorForObject, err)

-- it will fail for all objects simultaneously, but some comment would still be nice, like "we can receive only 3 types of error, thus ignoring is safe".

`processObject` can return errors not only from container build, but also because some nodes are unavailable. Can you give a list of errors (not full, just some groups) which we will ignore here? It seems everything is ok, the last error is https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/681b2c5fd45f3c6763db2be3509575d3c3f967db/pkg/services/policer/check.go#L46 -- it will fail for all objects simultaneously, but some comment would still be nice, like "we can receive only 3 types of error, thus ignoring is safe".

fyrchik commented

2024-01-16 16:13:36 +00:00

Maybe even do this in the commit message.

👍 1

dstepanov-yadro approved these changes 2024-01-22 15:21:59 +00:00

fyrchik reviewed 2024-01-23 07:20:37 +00:00

pkg/services/policer/process.go Outdated

					
				@ -73,0 +79,4 @@

				func skipLog(errs []string, err string) bool {

					for _, strErr := range errs {

						if strErr == err {

fyrchik commented

2024-01-23 07:20:34 +00:00

Are you sure this will work correctly? For pointer newly created pointer types (consider fmt.Errorf) we will have false here.

Are you sure this will work correctly? For pointer newly created pointer types (consider `fmt.Errorf`) we will have `false` here.

elebedeva commented

2024-01-30 08:33:21 +00:00

fixed

fyrchik reviewed 2024-01-23 08:08:14 +00:00

pkg/services/object/ape/checker.go Outdated

					
				@ -51,3 +51,1 @@

				var (

					errMissingOID = fmt.Errorf("object ID is not set")

				)

				var errMissingOID = fmt.Errorf("object ID is not set")