Skip to content

[raft] Protobuf encoding of proposals - #1765

Closed
MariemBaccari wants to merge 1 commit into
interuss:masterfrom
Orbitalize:proposal_binary_encoding
Closed

MariemBaccari wants to merge 1 commit into
interuss:masterfrom
Orbitalize:proposal_binary_encoding

Conversation

@MariemBaccari

@MariemBaccari MariemBaccari commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

This PR extracts the work of #1587 to implement protobuf based encoding of the proposal envelope (part of #1709). The previous PR used binary encoding but protobuf is a simplification that only adds a few bytes to each payload.
Follow-up PRs will implement compression and custom encoding for the payload values which will further reduce the payload size.

A small experiment shows the size difference between the json and protobuf encodings:

Case Cells Payload (B) JSON envelope (B) Protobuf envelope (B) Saved
RID delete ISA - 75 276 145 47.5%
SCD delete OIR - 213 461 285 38.2%
RID create ISA 1 275 544 346 36.4%
RID create ISA 10 455 784 526 32.9%
RID create ISA 100 2255 3184 2326 26.9%
RID create ISA 1000 20255 27184 20327 25.2%
RID create subscription 1 310 579 368 36.4%
RID create subscription 10 490 819 548 33.1%
RID create subscription 100 2290 3219 2348 27.1%
RID create subscription 1000 20290 27219 20349 25.2%
SCD create subscription 1 384 675 442 34.5%
SCD create subscription 10 564 915 622 32.0%
SCD create subscription 100 2364 3315 2422 26.9%
SCD create subscription 1000 20364 27315 20423 25.2%
SCD create constraint 1 296 566 361 36.2%
SCD create constraint 10 476 806 541 32.9%
SCD create constraint 100 2276 3206 2341 27.0%
SCD create constraint 1000 20276 27206 20342 25.2%
SCD create OIR 1 466 801 538 32.8%
SCD create OIR 10 646 1041 718 31.0%
SCD create OIR 100 2446 3441 2518 26.8%
SCD create OIR 1000 20446 27441 20519 25.2%

@MariemBaccari
MariemBaccari force-pushed the proposal_binary_encoding branch 2 times, most recently from d9a1f8f to 0fa1ec8 Compare October 8, 2026 10:01
@MariemBaccari
MariemBaccari marked this pull request as ready for review October 8, 2026 12:41
@MariemBaccari
MariemBaccari marked this pull request as draft October 8, 2026 13:35
@MariemBaccari
MariemBaccari force-pushed the proposal_binary_encoding branch 7 times, most recently from dc1f2e9 to e09b2bb Compare October 9, 2026 09:34
@MariemBaccari MariemBaccari changed the title [raft] Binary encoding proposals [raft] Protobuf encoding of proposals Oct 9, 2026
@MariemBaccari
MariemBaccari marked this pull request as ready for review October 9, 2026 11:50
@MariemBaccari
MariemBaccari force-pushed the proposal_binary_encoding branch from e09b2bb to 1f46e33 Compare October 9, 2026 12:25
@MariemBaccari
MariemBaccari marked this pull request as draft October 9, 2026 12:47
@MariemBaccari
MariemBaccari force-pushed the proposal_binary_encoding branch from 1f46e33 to 47af5a5 Compare October 9, 2026 12:51
@MariemBaccari
MariemBaccari marked this pull request as ready for review October 12, 2026 06:45
Comment thread build/protoc-go/Dockerfile Resolved
Comment thread build/protoc-go/Dockerfile Resolved
Comment thread pkg/raftstore/README.md Resolved
Comment thread build/protoc-go/Dockerfile Outdated
@@ -0,0 +1,23 @@
FROM golang:1.27.1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider passing the go version as an argument from the makefile:

  • in general we will this version to be >= version in the go.mod
  • there are several other places where a go version is specified that would be good to be kept in sync (e.g. openapi-to-go-server, test-certs, dummy-oauth, etc.). Not necessarily in this PR, but that would at least prepare the ground work for that (drop a TODO comment if not implemented in this PR)

return Proposal{
ID: id.String(),
NodeID: pb.GetNodeId(),
Timestamp: pb.GetTimestamp().AsTime(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked the validity of this regarding timezones?
IIUC tz info is lost after decoding here, but it might be irrelevant (e.g. always set to UTC, tz are never taken into account, etc.)
In any case: it is worth a check and an explicit clarification somewhere somehow. Especially since I expect different nodes to operate in different timezones in our case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added a comment to clarify that the timestamp is always set to UTC to avoid this 👍

@MariemBaccari

Copy link
Copy Markdown
Contributor Author

This feedback under #1775 should be addressed in this PR.

@MariemBaccari
MariemBaccari force-pushed the proposal_binary_encoding branch 2 times, most recently from ddfc59e to bad1496 Compare October 27, 2026 07:52
@MariemBaccari
MariemBaccari force-pushed the proposal_binary_encoding branch from bad1496 to 6637975 Compare October 27, 2026 08:02
@MariemBaccari
MariemBaccari requested a review from mickmis October 27, 2026 08:03
mickmis
mickmis previously approved these changes Oct 27, 2026

@mickmis mickmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mickmis
mickmis dismissed their stale review October 27, 2026 12:24

Second thoughts

@mickmis mickmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline:

  • actually define the payload as a proto message with a oneof replacing request_type and value
  • proto files in interfaces root folder

Example:

syntax = "proto3";

package dss.raftstore.consensus;

import "google/protobuf/timestamp.proto";

option go_package = "github.com/interuss/dss/pkg/raftstore/consensus/proposalpb";

// in interfaces

message UpsertOperationalIntent {
    string id = 1;
    string manager = 2;
    // etc. ...
}

message SearchOperationalIntents {
    google.protobuf.Timestamp start = 2;
    // etc. ...
}

message Proposal {
    bytes id = 1;
    uint64 node_id = 2;
    google.protobuf.Timestamp timestamp = 3;
    oneof payload {
        UpsertOperationalIntent UpsertOperationalIntent = 10;
        SearchOperationalIntents searchOperationalIntents = 11;
        // etc.
    }
//    string request_type = 4; // replaced by payload
//    bytes value = 5; // replaced by payload
//    bool read_only = 6; // could be inferred replaced by payload maybe?
}

@MariemBaccari

Copy link
Copy Markdown
Contributor Author

Considering @mickmis 's feedback, I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants