Skip to content

feat(meta): avoid sending inject barrier request to all workers#24361

Merged
wenym1 merged 2 commits intomainfrom
yiming/avoid-send-inject-barrier-to-all-nodes
Jan 13, 2026
Merged

feat(meta): avoid sending inject barrier request to all workers#24361
wenym1 merged 2 commits intomainfrom
yiming/avoid-send-inject-barrier-to-all-nodes

Conversation

@wenym1
Copy link
Contributor

@wenym1 wenym1 commented Jan 6, 2026

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Previously we had to send InjectBarrierRequest to all worker nodes, even if there is no actors on the nodes. We implement it in this way because: 1. we used to broadcast some global information via the request, and 2. there was a mysterious panic in CI.

For 1, we don't broadcast anything in the request anymore. For 2, the mysterious panic was because of the following. We have two important fields in InjectBarrierRequest, actor_ids_to_collect and table_ids_to_sync. When handling a command, we have two steps, pre_apply and post_apply. actor_ids_to_collect should include the remaining actors between pre_apply and post_apply, while table_ids_to_sync should only include the table_ids after post_apply. Therefore, for example, when handling a drop command, the dropped fragments still exist after pre_apply, and have been removed after post_apply, and then the actors of the fragments should be included in the actor_ids_to_collect, while the table_ids should not be included in table_ids_to_sync.

To handle this, in this PR, we additionally pass a param nodes_to_sync_table in inject_barrier. For a node that has actors on it, but not in nodes_to_sync_table, we will set an empty table_ids_to_sync in InjectBarrierRequest.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

  • My PR needs documentation updates.
Release note

Copy link
Contributor Author

wenym1 commented Jan 6, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@wenym1 wenym1 force-pushed the yiming/avoid-send-inject-barrier-to-all-nodes branch from 652545b to ec342f4 Compare January 7, 2026 06:56
Copy link
Member

@yezizp2012 yezizp2012 left a comment

Choose a reason for hiding this comment

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

LGTM

generate_series,
'{"orders": {"id": 1, "price": "2.30", "customer_id": 2}}'::jsonb
FROM generate_series(1, 50000);
FROM generate_series(1, 10000);
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for the modification here? To decrease the CI time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, without it, the ci backfill test will be timed out.

@wenym1 wenym1 added this pull request to the merge queue Jan 13, 2026
Merged via the queue into main with commit 3f6f6df Jan 13, 2026
36 of 38 checks passed
@wenym1 wenym1 deleted the yiming/avoid-send-inject-barrier-to-all-nodes branch January 13, 2026 05:27
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