[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.

[MariemBaccari]

And #1716 for generalizing the method :)

[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.

[MariemBaccari]

I opened PRs #1711, #1712, #1713 and #1714 to precompute S2 cells for SCD. I also updated the future PRs of this chain to reflect the change.
Part of the change actually includes changing the Repository interface to accept cells instead of 4D volumes and only calculating the S2 cells covering once at the http handler level. Given the fact that the business logic is shared between crdb and raftstore (which executes it against the memstore), it is important that the memstore implements the repository interface. Otherwise, it wouldn't be able to execute the operations. What do you think ?

[mickmis]

I was pretty sure it was OK for memstore to drop implementing the interface since it is used exclusively by the raftstore, but I may remember incorrectly.
Now in itself moving up the stack the S2 cells covering calculation is a good thing anyway, so we can postpone that discussion, if ever we need to have it.

[MariemBaccari]

It was indeed ok when we used to duplicate the business logic code for the apply path. Now that both crdb and raftstore use the same path, we still need to pass a repository to it. I agree with you on the S2 cells part :)

[mickmis]

LGTM