Skip to content

Commit 79cbfb9

Browse files
go: fix channel auto coalescing in merge (#1322)
### Changelog <!-- Write a one-sentence summary of the user-impacting change (API, UI/UX, performance, etc) that could appear in a changelog. Write "None" if there is no user-facing change --> Ensure channel auto coalescing is deterministic in `mcap merge` ### Docs <!-- Link to a Docs PR, tracking ticket in Linear, OR write "None" if no documentation changes are needed. --> ### Description In the `mcap merge` command, the channel auto coalescing is currently non-deterministic due to the random ordering of iterating over a map in go. This change just sorts the metadata keys before we hash it to ensure for the same channel metadata we always get the same hash. For example, if trying to merge these to mcaps ``` library: mcap go v1.7.1; mcap go v1.7.0 profile: messages: 800 duration: 19.979441843s start: 2024-07-01T10:23:52.441156385-07:00 (1719854632.441156385) end: 2024-07-01T10:24:12.420598228-07:00 (1719854652.420598228) compression: zstd: [1/1 chunks] [273.00 KiB/36.78 KiB (86.53%)] [1.84 KiB/sec] channels: (4) /imu 800 msgs (40.04 Hz) : sensor_msgs/Imu [ros1msg] channels: 1 attachments: 0 metadata: 0 ``` ``` library: mcap go v1.7.1; mcap go v1.7.0 profile: messages: 800 duration: 19.979921296s start: 2024-07-01T15:05:18.571434739-07:00 (1719871518.571434739) end: 2024-07-01T15:05:38.551356035-07:00 (1719871538.551356035) compression: zstd: [1/1 chunks] [273.00 KiB/35.08 KiB (87.15%)] [1.75 KiB/sec] channels: (4) /imu 800 msgs (40.04 Hz) : sensor_msgs/Imu [ros1msg] channels: 1 attachments: 0 metadata: 0 ``` will sometimes give this ```bash $ mcap merge imu0.mcap imu1.mcap -o test.mcap && mcap info test.mcap library: mcap go v1.7.1 profile: messages: 1600 duration: 4h41m46.11019965s start: 2024-07-01T10:23:52.441156385-07:00 (1719854632.441156385) end: 2024-07-01T15:05:38.551356035-07:00 (1719871538.551356035) compression: zstd: [1/1 chunks] [543.32 KiB/67.37 KiB (87.60%)] [4.00 B/sec] channels: (1) /imu 1600 msgs (0.09 Hz) : sensor_msgs/Imu [ros1msg] channels: 1 attachments: 0 metadata: 0 ``` and sometimes give this ```bash mcap merge imu0.mcap imu1.mcap -o test.mcap && mcap info test.mcap library: mcap go v1.7.1 profile: messages: 1600 duration: 4h41m46.11019965s start: 2024-07-01T10:23:52.441156385-07:00 (1719854632.441156385) end: 2024-07-01T15:05:38.551356035-07:00 (1719871538.551356035) compression: zstd: [1/1 chunks] [543.45 KiB/67.85 KiB (87.51%)] [4.00 B/sec] channels: (1) /imu 800 msgs (0.05 Hz) : sensor_msgs/Imu [ros1msg] (2) /imu 800 msgs (0.05 Hz) : sensor_msgs/Imu [ros1msg] channels: 2 attachments: 0 metadata: 0 ``` I've updated the unit tests to test for this. Fixes: foxglove/repo#772
1 parent b80d5bd commit 79cbfb9

File tree

2 files changed

+19
-7
lines changed

2 files changed

+19
-7
lines changed

go/cli/mcap/cmd/merge.go

+10-2
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"fmt"
1111
"io"
1212
"os"
13+
"slices"
1314

1415
"github.com/foxglove/mcap/go/cli/mcap/utils"
1516
"github.com/foxglove/mcap/go/mcap"
@@ -157,9 +158,16 @@ func getChannelHash(channel *mcap.Channel, coalesceChannels string) HashSum {
157158

158159
switch coalesceChannels {
159160
case AutoCoalescing: // Include channel metadata in hash
160-
for key, value := range channel.Metadata {
161+
// sort keys so we get same metadata order in the hash
162+
keys := make([]string, 0, len(channel.Metadata))
163+
for key := range channel.Metadata {
164+
keys = append(keys, key)
165+
}
166+
slices.Sort(keys)
167+
168+
for _, key := range keys {
161169
hasher.Write([]byte(key))
162-
hasher.Write([]byte(value))
170+
hasher.Write([]byte(channel.Metadata[key]))
163171
}
164172
case ForceCoalescing: // Channel metadata is not included in hash
165173
break

go/cli/mcap/cmd/merge_test.go

+9-5
Original file line numberDiff line numberDiff line change
@@ -537,26 +537,30 @@ func TestSameSchemasNotDuplicated(t *testing.T) {
537537

538538
func TestChannelCoalesceBehavior(t *testing.T) {
539539
expectedMsgCountByChannel := map[string]map[uint16]int{
540-
"none": {1: 100, 2: 100, 3: 100, 4: 100},
541-
"auto": {1: 200, 2: 100, 3: 100},
542-
"force": {1: 300, 2: 100},
540+
"none": {1: 100, 2: 100, 3: 100, 4: 100, 5: 100},
541+
"auto": {1: 200, 2: 200, 3: 100},
542+
"force": {1: 400, 2: 100},
543543
}
544544

545545
for coalesceChannels, messagesByChannel := range expectedMsgCountByChannel {
546546
buf1 := &bytes.Buffer{}
547547
buf2 := &bytes.Buffer{}
548548
buf3 := &bytes.Buffer{}
549549
buf4 := &bytes.Buffer{}
550+
buf5 := &bytes.Buffer{}
551+
fooMetadata := map[string]string{"k0": "v", "k1": "v", "k2": "v", "k3": "v"}
550552
prepInput(t, buf1, &mcap.Schema{ID: 1}, &mcap.Channel{ID: 1, Topic: "/foo"})
551553
prepInput(t, buf2, &mcap.Schema{ID: 1}, &mcap.Channel{ID: 2, Topic: "/foo"})
552-
prepInput(t, buf3, &mcap.Schema{ID: 1}, &mcap.Channel{ID: 3, Topic: "/foo", Metadata: map[string]string{"k": "v"}})
553-
prepInput(t, buf4, &mcap.Schema{ID: 1}, &mcap.Channel{ID: 4, Topic: "/bar"})
554+
prepInput(t, buf3, &mcap.Schema{ID: 1}, &mcap.Channel{ID: 3, Topic: "/foo", Metadata: fooMetadata})
555+
prepInput(t, buf4, &mcap.Schema{ID: 1}, &mcap.Channel{ID: 4, Topic: "/foo", Metadata: fooMetadata})
556+
prepInput(t, buf5, &mcap.Schema{ID: 1}, &mcap.Channel{ID: 5, Topic: "/bar"})
554557
output := &bytes.Buffer{}
555558
inputs := []namedReader{
556559
{"buf1", bytes.NewReader(buf1.Bytes())},
557560
{"buf2", bytes.NewReader(buf2.Bytes())},
558561
{"buf3", bytes.NewReader(buf3.Bytes())},
559562
{"buf4", bytes.NewReader(buf4.Bytes())},
563+
{"buf5", bytes.NewReader(buf5.Bytes())},
560564
}
561565
merger := newMCAPMerger(mergeOpts{coalesceChannels: coalesceChannels, allowDuplicateMetadata: true})
562566
require.NoError(t, merger.mergeInputs(output, inputs))

0 commit comments

Comments
 (0)