[raft/scd] Implement constraints repo methods

[MariemBaccari]

Implements the scd constraints repo methods for the raftstore. The raftstore repo methods issue individual proposals.

Implements #1700

[mickmis]

This prompted me to check something with the implementation of HandleClientRequest , and I notice that we do this:

type Proposal struct {
	ID          string      `json:"id"`
	Locality    string      `json:"locality"`
	NodeID      uint64      `json:"node_id"`
	Timestamp   time.Time   `json:"timestamp"`
	RequestType RequestType `json:"request_type"`
	Value       []byte      `json:"value"`
	// ReadOnly proposals do not modify the state machine and,
	// therefore, do not need to be applied by nodes who did not initiate them.
	// TODO: This is a temporary solution. In the future, we will use ReadIndex
	// for read-only operations without needing to propose them to Raft.
	ReadOnly bool `json:"read_only"`
}

func (c *Consensus) HandleClientRequest(ctx context.Context, requestType RequestType, value []byte, readOnly bool) (any, error) {
	proposal := c.newProposal(ctx, requestType, value, readOnly)
	buf, err := json.Marshal(proposal)
...

I.e. we call json.Marshal on the payload of the proposal once, and then a second time when marshalling the proposal.

Thing is, Value will be base64-encoded in that case:

Array and slice values encode as JSON arrays, except that []byte encodes as a base64-encoded string, and a nil slice encodes as the null JSON value.

Which will waste some space I believe and will be less readable.

To avoid that Value may be set to use the type json.RawMessage, see the doc: https://pkg.go.dev/encoding/json#RawMessage

If I am correct, could you address that in a separate PR? Thanks!

[MariemBaccari]

@mickmis Thanks for bringing this up. I will add it as a TODO on the PR train document and see how to deal with this depending on how exactly we will be optimizing the proposal encoding.

[mickmis]

(fb30ae4)

[mickmis]

By upgrading to go 1.27 and using generic methods (on HandleClientRequest) I think we can depuplicate a fair bit of code. At least this could be looking like:

	return r.consensus.HandleClientRequest(ctx, getConstraint, buf, true)

Given:

type RequestType[Result any] string
func (c *Consensus) HandleClientRequest[Result any](ctx context.Context, requestType RequestType[Result], value []byte, readOnly bool) (Result, error) {

It may even be worth doing now, but not sure (1.27 upgrade is relatively painless, mostly it would be the small refactoring described above). WDYT?

[MariemBaccari]

That sounds good, I'll look into the upgrade and get back to you soon.

[MariemBaccari]

Opened PR #1698 for the upgrade.

[mickmis]

I was about to suggest not introducing those new abstractions to work around the Geometry interface, but actually the reason we have this is that we should be only caring about the s2 cells the geometry is touching. And given our current discussions about not propagating the full geometry, do we agree that once we do that we do not need anymore those marshalling-specific abstractions?
If yes, and if we indeed decide the v1 should not propagate the geometries, what would you think of first implementing this?

[mickmis]

After having made some investigations, FWIW this will mean that the memstore will not be implementing anymore the Repository interface. But that's IMO OK (I had actually suggested to @the-glu at some point to drop this self-imposed constraint, but it did not happen in the end; my point here is that implementing this interface is not mandatory).

[MariemBaccari]

Agreed, I'm just fixing the design doc now to include the S2 cells in proposals (the reasons to go forward with that just keep on piling up haha). I'll then make a PR and it should indeed help us get rid of this.