Make quorum functions always take the request as a parameter

[meling]

Currently, if the quorum function needs access to the request object, the developer needs to provide an option gorums.qf_with_req to the RPC method for which this applies. This has been found to be a bit confusing and newcomers may find it awkward to work with different signatures for the different quorum functions. Moreover, it is believed that the overhead of passing one extra parameter that is not used by quorum function is low, and thus it is reasonable to instead always require that the quorum function follows the same signature, i.e. that the first argument is the request object and the second argument is the set of replies.

[meling]

As an experiment, I implemented a solution with reflection, that can support quorum functions with or without the request argument. I don't think its a clean and clear enough solution, so I prefer not to adopt this, but I add the code here for reference:

So instead of

if resp, quorum = c.qspec.WriteQF(a, replyValues); quorum {
	return resp, nil
}

We can do this:

m := reflect.ValueOf(c.qspec).MethodByName("WriteQF")
if m.IsValid() {
	in := make([]reflect.Value, 0)
	if m.Type().In(0) == reflect.TypeOf(a) {
		in = append(in, reflect.ValueOf(a))
	}
	in = append(in, reflect.ValueOf(replyValues))
	out := m.Call(in)
	if out[1].Bool() {
		return out[0].Interface().(*WriteResponse), nil
	}
} else {
	panic("method not found: WriteQF")
}

Technically, it should be safe enough since we would be generating this code. However, we would not adhere to the QuorumSpec interface, and we could also get a runtime panic if the developer does not implement the correct method. Though this is likely to be discovered before any deployment. Obviously, it is also more costly, but I didn't do any performance analysis.

On the other hand, we could actually remove the whole QuorumSpec interface, but require that some interface{} type is provided that implements either WriteQF(req, replies) or WriteQF(replies). The drawback is that we lose compile time checking that at least one of these functions exists.

My conclusion is that we should just discard the qf_with_req option to make things simple and always pass the request argument.

[meling]

Commit 673ba31 on branch remove-req-param-optional removes the qf_with_req option. However, I'm getting test failure on:

--- FAIL: TestQuorumCallCancel (0.00s)
        config_qc_test.go:503: got error: quorum call error: context canceled (errors: 1, replies: 0), want: quorum call error: context canceled (errors: 0, replies: 0)

The number of errors seems to vary from 1, 2, 3 across different runs. I can't see why exactly this should be related to my change, but at the same time, I'm not getting the same failure on the master branch.

[meling]

Correction: I'm also getting the same failure on master, but it is not always easy to reproduce. My current investigation seems to indicate that the introduction of 4dd8595 caused this; that is, by removing the wg.Wait() this problem appears to go away in both master and remove-req-param-optional branches. However, it is not a reasonable solution to avoid the workgroup thing just because of this. @tormoder is there something else we could do?

I'm wondering if we should be counting "context canceled" errors or if those are different?

[tormoder]

Moved test failure to a separate issue since it is unrelated.

[tormoder]

Regarding the reflection snippet: Since we are using code generation, we should not need to use reflection in general.

[meling]

@tormoder the problem with generating in this particular case is that we need to signal to the generator whether or not to generate one or the other function signature, and that extra cognitive load we should avoid. Even if it doesn't feel too much right now, it may cause a headache later.

Anyway, I guess we agree that we shouldn't pursue the reflection based approach.

Then the question becomes: should we make it the default behavior to always pass the request argument to the quorum function??

[tormoder]

In general I think the request parameter is useful for consensus protocols that uses the slot+ballot/round concept, since it is often needed to the check the reply round against the round you sent (e.g. if the reply contains a newer round).

For register/storage algorithms I think it is not that relevant, but maybe @leandernikolaus can comment on that.

I would prefer turning it on when need, but also okay with having it as a default.

[meling]

Closing, since passing the request object will be the default in the upcoming rewrite. There is no performance benefit to be had with this option.