Skip to content

Conversation

VolodymyrBg
Copy link
Contributor

Replace index-based deletion with stable id matching in Producer.Start and assign ids from a monotonically increasing counter in Subscribe. This fixes incorrect removals after slice compaction while keeping behavior otherwise unchanged.

@eljobe eljobe self-requested a review September 9, 2025 05:50
@eljobe eljobe self-assigned this Sep 9, 2025
@eljobe
Copy link
Member

eljobe commented Sep 9, 2025

Hey @VolodymyrBg, can you provide a testcase that would be failing before this change?
I haven't seen the issue you describe in the PR comment.

@eljobe eljobe assigned VolodymyrBg and unassigned eljobe Sep 9, 2025
@VolodymyrBg
Copy link
Contributor Author

Hey @VolodymyrBg, can you provide a testcase that would be failing before this change? I haven't seen the issue you describe in the PR comment.

added a test case

@eljobe eljobe assigned eljobe and unassigned VolodymyrBg Sep 9, 2025
@eljobe
Copy link
Member

eljobe commented Sep 9, 2025

Hey @VolodymyrBg, can you provide a testcase that would be failing before this change? I haven't seen the issue you describe in the PR comment.

added a test case

This looks very strange. Usually go tests go in a separate _test.go file.

@eljobe eljobe assigned VolodymyrBg and unassigned eljobe Sep 9, 2025
@VolodymyrBg
Copy link
Contributor Author

Hey @VolodymyrBg, can you provide a testcase that would be failing before this change? I haven't seen the issue you describe in the PR comment.

added a test case

This looks very strange. Usually go tests go in a separate _test.go file.

sh... really did a bad thing and didn't even notice this, sorry

@VolodymyrBg
Copy link
Contributor Author

Hey @VolodymyrBg, can you provide a testcase that would be failing before this change? I haven't seen the issue you describe in the PR comment.

added a test case

This looks very strange. Usually go tests go in a separate _test.go file.

it was my copypaste mistake

@eljobe eljobe assigned eljobe and unassigned VolodymyrBg Sep 11, 2025
Copy link
Member

@eljobe eljobe left a comment

Choose a reason for hiding this comment

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

Thanks for the test, and the fix. This is clearly better.

Can you also switch your indentation in the test file to use tabs instead of spaces?

@eljobe eljobe assigned VolodymyrBg and unassigned eljobe Sep 11, 2025
@VolodymyrBg
Copy link
Contributor Author

Thanks for the test, and the fix. This is clearly better.

Can you also switch your indentation in the test file to use tabs instead of spaces?

Corrected

@eljobe eljobe assigned eljobe and unassigned VolodymyrBg Sep 26, 2025
@eljobe eljobe enabled auto-merge September 26, 2025 07:26
@eljobe eljobe added this pull request to the merge queue Sep 26, 2025
auto-merge was automatically disabled September 26, 2025 08:06

Pull Request is not mergeable

Merged via the queue into OffchainLabs:master with commit b580dd7 Sep 26, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants