[#270] Add IR epoch tick control call #282

ale64bit · 2023-04-25T12:14:54Z

ale64bit commented

2023-04-25 12:14:54 +00:00

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

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

ale64bit requested review from storage-core-committers 2023-04-25 12:15:09 +00:00

ale64bit requested review from storage-core-developers 2023-04-25 12:15:10 +00:00

dstepanov-yadro reviewed 2023-04-25 13:59:49 +00:00

cmd/frostfs-cli/modules/control/ir_tick_epoch.go Outdated

					
				@ -0,0 +12,4 @@

				var tickEpochCmd = &cobra.Command{

					Use:   "tick-epoch",

					Short: "Forces a new epoch",

					Long:  "Forces a new epoch. Note that it is not guaranteed to succeed, although it will with high probability.",

dstepanov-yadro commented

2023-04-25 13:59:49 +00:00

:)
Can we add the probability value? (It is a joke)

:) Can we add the probability value? (It is a joke)

ale64bit commented

2023-04-25 16:04:23 +00:00

Well :) it's not a constant value anyway. But >95%, so likely enough.

fyrchik commented

2023-04-26 10:16:30 +00:00

This command sends a notary request, so it always succeeds.
Can we mention this and that the command must be executed on other IR nodes?

This command sends a notary request, so it always succeeds. Can we mention this and that the command must be executed on other IR nodes?

ale64bit commented

2023-04-26 10:49:24 +00:00

done

fyrchik marked this conversation as resolved

dstepanov-yadro commented

2023-04-25 14:05:36 +00:00

Why don't use frostfs-adm force new epoch command?

ale64bit commented

2023-04-25 16:05:21 +00:00

Why don't use frostfs-adm force new epoch command?

There's more info about it in the original issue, including this very same question: https://github.com/nspcc-dev/neofs-node/issues/1866#issuecomment-1273774642.

> Why don't use frostfs-adm force new epoch command? There's more info about it in the original issue, including this very same question: https://github.com/nspcc-dev/neofs-node/issues/1866#issuecomment-1273774642.

👍 1

dstepanov-yadro approved these changes 2023-04-26 06:15:01 +00:00

acid-ant approved these changes 2023-04-26 07:01:55 +00:00

fyrchik reviewed 2023-04-26 10:16:41 +00:00

pkg/innerring/processors/netmap/handlers.go Outdated

					
				@ -16,7 +16,7 @@ func (np *Processor) HandleNewEpochTick(ev event.Event) {

					// send an event to the worker pool

					err := np.pool.Submit(func() { np.processNewEpochTick() })

fyrchik commented

2023-04-26 10:10:36 +00:00

Unrelated to the commit

ale64bit commented

2023-04-26 10:49:33 +00:00

done

fyrchik marked this conversation as resolved

pkg/morph/client/notary.go Outdated

					
				@ -907,0 +908,4 @@

					// probability of all nodes producing the same transaction, since it depends

					// on this value.

					if controlTx {

						const minInc = 100

fyrchik commented

2023-04-26 10:14:14 +00:00

Using MaxValidUntilBlockIncrement here will achieve the same with higher probability.

Using `MaxValidUntilBlockIncrement` here will achieve the same with higher probability.

ale64bit commented

2023-04-26 10:49:40 +00:00

done

fyrchik marked this conversation as resolved

pkg/morph/client/static.go Outdated

					
				@ -106,1 +111,4 @@

				// SetControlTX sets whether a control transaction will be used.

				func (i *InvokePrmOptional) SetControlTX(b bool) {

					i.controlTX = b

fyrchik commented

2023-04-26 10:14:43 +00:00

What about using calculateVUB as a parameter instead of less descriptive controlTX?

What about using `calculateVUB` as a parameter instead of less descriptive `controlTX`?

ale64bit commented

2023-04-26 10:50:08 +00:00

discussed offline. Renamed to roundBlockHeight and added additional comment.

discussed offline. Renamed to `roundBlockHeight` and added additional comment.

fyrchik marked this conversation as resolved

pkg/services/control/ir/convert.go Outdated

					
				@ -16,3 +16,2 @@

				type healthCheckResponseWrapper struct {

					m *HealthCheckResponse

				type responseWrapper[M grpc.Message] struct {

fyrchik commented

2023-04-26 10:11:39 +00:00

Unrelated to the commit

ale64bit commented

2023-04-26 10:50:20 +00:00

It's used by sendUnary.

It's used by `sendUnary`.

fyrchik marked this conversation as resolved

pkg/services/control/ir/server/calls.go Outdated

					
				@ -35,0 +38,4 @@

				//

				// If request is not signed with a key from white list, permission error returns.

				func (s *Server) TickEpoch(_ context.Context, req *control.TickEpochRequest) (*control.TickEpochResponse, error) {

					// verify request

fyrchik commented

2023-04-26 10:12:39 +00:00

We have this such comments in code, but IMO they are not needed, especially when the function is called isValidRequest

We have this such comments in code, but IMO they are not needed, especially when the function is called `isValidRequest`

ale64bit commented

2023-04-26 10:50:29 +00:00

done

fyrchik marked this conversation as resolved

ale64bit force-pushed feature/270-cmd-control-ir from 0d978bb21d to 0ba646aedc

2023-04-26 10:49:21 +00:00

Compare

carpawell reviewed 2023-04-26 16:08:09 +00:00

carpawell left a comment

@fyrchik, dont we want to search for already prepared TX in mempool (or how it is named for notary service) via some neo-go RPC (does it exists?)? it would sound more naturally to me: only the first Alphabet node inits notary request while the others just look for it and add their signatures, and not try to predict how the common for all nodes TX should look (via rounding and so on)

@fyrchik, dont we want to search for already prepared TX in mempool (or how it is named for notary service) via some `neo-go` RPC (does it exists?)? it would sound more naturally to me: only the first Alphabet node inits notary request while the others just look for it and add their signatures, and not try to predict how the _common for all nodes_ TX should look (via rounding and so on)

fyrchik requested review from carpawell 2023-04-27 08:20:30 +00:00

fyrchik commented

2023-04-27 08:24:01 +00:00

Mempool is a hash-map, how do you want to search it?
In future we could listen for a notary event and remember some txs in the control service, "approving" them with a separate command.

Mempool is a hash-map, how do you want to search it? In future we could listen for a notary event and remember some txs in the control service, "approving" them with a separate command.

fyrchik reviewed 2023-04-27 10:30:58 +00:00

pkg/morph/client/netmap/new_epoch.go Outdated

					
				@ -9,3 +9,3 @@

				// NewEpoch updates FrostFS epoch number through

				// Netmap contract call.

				func (c *Client) NewEpoch(epoch uint64) error {

				// If `force` is true, this call is normally initiated by a control

fyrchik commented

2023-04-27 10:29:39 +00:00

Why is it called force then?

Why is it called `force` then?

ale64bit commented

2023-04-27 10:48:55 +00:00

what should it be called?

pkg/morph/client/notary.go Outdated

					
				@ -290,3 +290,3 @@

					}

					nonce, vub, err := c.CalculateNonceAndVUB(prm.hash)

					nonce, vub, err := c.CalculateNonceAndVUB(prm.hash, false)

fyrchik commented

2023-04-27 10:30:44 +00:00

What about a separate function? Like CalculateNonceAndVUBControl?
It serves 1 use-case only and it is not immediately obvious what is the meaning of false/true when reading code.

What about a separate function? Like `CalculateNonceAndVUBControl`? It serves 1 use-case only and it is not immediately obvious what is the meaning of `false`/`true` when reading code.

ale64bit commented

2023-04-27 10:48:45 +00:00

done

fyrchik marked this conversation as resolved

ale64bit force-pushed feature/270-cmd-control-ir from 0ba646aedc to af7e288e4a

2023-04-27 10:48:27 +00:00

Compare

fyrchik approved these changes 2023-04-28 07:56:50 +00:00

fyrchik merged commit ff25521204 into master

2023-04-28 07:57:02 +00:00

Sign in to join this conversation.

No reviewers