[#92] Refactor policer and add some unit tests #485

ale64bit · 2023-06-29T09:18:19Z

ale64bit commented

2023-06-29 09:18:19 +00:00

Signed-off-by: Alejandro Lopez a.lopez@yadro.com

Signed-off-by: Alejandro Lopez <a.lopez@yadro.com>

🎉 1

ale64bit requested review from storage-core-committers 2023-06-29 09:18:36 +00:00

ale64bit requested review from storage-core-developers 2023-06-29 09:18:36 +00:00

fyrchik approved these changes 2023-06-29 15:03:52 +00:00

pkg/services/policer/option.go Outdated

					
				@ -0,0 +30,4 @@

				type RedundantCopyCallback func(context.Context, oid.Address)

				// BuryFunc is the function to bury (i.e. inhume) an object.

				type BuryFunc func(context.Context, oid.Address) error

fyrchik commented

2023-06-29 14:59:24 +00:00

So BuryFunc is a function, but Replicator is a single-method interface.
What are the differences?

So `BuryFunc` is a function, but `Replicator` is a single-method interface. What are the differences?

ale64bit commented

2023-06-30 07:32:35 +00:00

I don't think there's a major difference, except where the wrapper will live. In this case, I tried to preserve the call-site option for Replicator by making it an interface in policer, while the bury function wasn't there in the first place so there was no need to wrap it in a struct/interface.

I don't think there's a major difference, except where the wrapper will live. In this case, I tried to preserve the call-site option for `Replicator` by making it an interface in policer, while the bury function wasn't there in the first place so there was no need to wrap it in a struct/interface.

👍 1

fyrchik marked this conversation as resolved

pkg/services/policer/option.go Outdated

					
				@ -0,0 +51,4 @@

					log *logger.Logger

					keySpaceIt KeySpaceIterator

fyrchik commented

2023-06-29 14:56:47 +00:00

Why It and not Iterator?

Why `It` and not `Iterator`?

ale64bit commented

2023-06-30 07:21:49 +00:00

It's a well-known abbreviation so it seems clear to shorten it.
Other fields use similar or less-known abbreviations, e.g. cfg, cnrSrc, cbRedundantCopy, rebalanceFreq` and so on.

It's a well-known abbreviation so it seems clear to shorten it. Other fields use similar or less-known abbreviations, e.g. `cfg, `cnrSrc`, `cbRedundantCopy`, `rebalanceFreq` and so on.

fyrchik commented

2023-06-30 13:00:39 +00:00

Ok, but I had some troubles with parsing -- it is also an anaphor and it is hard to tell the meaning without looking at the type.

Ok, but I had some troubles with parsing -- `it` is also an anaphor and it is hard to tell the meaning without looking at the type.

ale64bit commented

2023-07-03 06:43:32 +00:00

done

it is also an anaphor and it is hard to tell the meaning without looking at the type.

I don't think I've ever seen people writing variable names like iLikeToMoveItMoveIt or anything like that, where an It suffix might be ambiguous.

done > it is also an anaphor and it is hard to tell the meaning without looking at the type. I don't think I've ever seen people writing variable names like `iLikeToMoveItMoveIt` or anything like that, where an `It` suffix might be ambiguous.

😆 2

fyrchik commented

2023-07-03 06:58:43 +00:00

Ironially, we have ToMoveIt in the metabase code. I don't like it though.

Ironially, we have `ToMoveIt` in the metabase code. I don't like it though.

🚀 2

pkg/services/policer/policer.go Outdated

					
				@ -26,3 +13,2 @@

				type objectsInWork struct {

					m    sync.RWMutex

					sync.RWMutex

fyrchik commented

2023-06-29 14:41:18 +00:00

Why this change? Could you move it to a separate commit?

ale64bit commented

2023-06-30 07:29:53 +00:00

Why this change?

If the purpose of the struct is just to wrap a field and its lock, it seems cleaner to me to embed the lock to avoid the .mu stutter in methods.

Could you move it to a separate commit?

done

> Why this change? If the purpose of the `struct` is just to wrap a field and its lock, it seems cleaner to me to embed the lock to avoid the `.mu` stutter in methods. > Could you move it to a separate commit? done

fyrchik marked this conversation as resolved

pkg/services/policer/policer_test.go Outdated

					
				@ -0,0 +168,4 @@

								}

							}

							placementBuilder := func(cnr cid.ID, obj *oid.ID, p netmap.PlacementPolicy) ([][]netmap.NodeInfo, error) {

								if cnr == addr.Container() && obj != nil && *obj == addr.Object() {

fyrchik commented

2023-06-29 15:02:56 +00:00

We have cid.ID.Equals() for this purpose, which is better (see the comment to Equals). Same for oid.ID.

We have `cid.ID.Equals()` for this purpose, which is better (see the comment to `Equals`). Same for `oid.ID`.

ale64bit commented

2023-06-30 07:21:59 +00:00

done

fyrchik marked this conversation as resolved

pkg/services/policer/policer_test.go Outdated

					
				@ -0,0 +220,4 @@

								WithBuryFunc(buryFn),

								WithRedundantCopyCallback(func(_ context.Context, a oid.Address) {

									if a != addr {

										t.Errorf("unexpected redundant copy callback: a=%v", a)

fyrchik commented

2023-06-29 14:50:34 +00:00

Why not require.Equal?

Why not `require.Equal`?

ale64bit commented

2023-06-30 08:02:35 +00:00

done (TrueCloudLab/frostfs-sdk-go#101)

done (https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/issues/101)

fyrchik commented

2023-06-30 13:03:00 +00:00

No, I mean that we use https://github.com/stretchr/testify and not t.* directly across the codebase. Do we have a compelling reason to not be uniform here?

No, I mean that we use https://github.com/stretchr/testify and not `t.*` directly across the codebase. Do we have a compelling reason to not be uniform here?

ale64bit commented

2023-06-30 13:08:45 +00:00

but ... I did change it to:

require.True(t, eqAddr(a, addr), "unexpected redundant copy callback: a=%v", a)

What exactly do you propose?

but ... I did change it to: ``` require.True(t, eqAddr(a, addr), "unexpected redundant copy callback: a=%v", a) ``` What exactly do you propose?

fyrchik commented

2023-06-30 15:19:19 +00:00

I see, looked at the old version first.

fyrchik marked this conversation as resolved

pkg/services/policer/policer_test.go Outdated

					
				@ -0,0 +271,4 @@

					return ret, nil

				}

				// containerSrcFunc is a container.Source backed by a function

fyrchik commented

2023-06-29 14:45:06 +00:00

Dot at the end (here and below), tests are skipped by linter, godot would complain

ale64bit commented

2023-06-30 07:27:26 +00:00

done

fyrchik marked this conversation as resolved

pkg/services/policer/process.go Outdated

					
				@ -67,3 +54,1 @@

									p.cache.Add(addr.Address, time.Now())

									p.objsInWork.remove(addr.Address)

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

fyrchik commented

2023-06-29 14:55:40 +00:00

What was the problem here? Anyway, could you move this to a separate commit?

ale64bit commented

2023-06-30 07:28:45 +00:00

Seems to me that it's possible (in theory, I don't have a test case yet) for an object to be worked on concurrently, with enough bad luck and proper scheduler timing. This ensures it can't happen.

fyrchik marked this conversation as resolved

ale64bit force-pushed fix/policer-refactor-test from c1bdf2cf13 to 99c8b9e641

2023-06-30 07:17:11 +00:00

Compare

ale64bit force-pushed fix/policer-refactor-test from 99c8b9e641 to fb0d6d1e5d

2023-06-30 08:02:03 +00:00

Compare

ale64bit requested review from fyrchik 2023-06-30 08:02:39 +00:00

dstepanov-yadro approved these changes 2023-06-30 08:31:59 +00:00

ale64bit force-pushed fix/policer-refactor-test from fb0d6d1e5d to e7f3a9fc19

2023-07-03 06:42:03 +00:00

Compare