Change information stored in `_topic_node_index` to avoid oversized alloc #17350

ballard26 · 2024-03-22T22:00:08Z

_topic_node_index originally stored a bunch of metadata about every group a node led. However, this information was only ever used in even_topic_distributon_constraint::recommended_reassignment. A method which isn't used anywhere outside of unit tests.

This PR changes the information stored in _topic_node_index to be just the count of leaders on a given node. And removes the implementation of even_topic_distributon_constraint::recommended_reassignment.

Fixes #17349

Backports Required

Release Notes

none

vbotbuildovich · 2024-03-23T00:13:13Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46641#018e685e-435f-491b-bfd0-b1d51343add3

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48832#018f5921-de88-487b-a388-a72304535ec4

travisdowns · 2024-03-25T15:33:36Z

src/v/cluster/scheduling/leader_balancer_constraints.cc

-    }
-
-    return std::nullopt;
+    vassert(false, "not implemented");


Can you include a clear comment here about why this won't be called? Is it called on other types implementing this interface?

@ballard26 this seems to have introduced a CI failure on v24.1.x: https://buildkite.com/redpanda/redpanda/builds/51764#0190c949-1a0f-42ba-a040-4e33bc85fc38

ERROR 2024-07-19 07:45:03,854 [shard 0:main] assert - Assert failure: (/var/lib/buildkite-agent/builds/buildkite-amd64-builders-i-0269b6a11fdecc85f-1/redpanda/redpanda/src/v/cluster/scheduling/leader_balancer_constraints.cc:59) 'false' not implemented

thanks @BenPope looks like something changed, or the assumptions weren't correct initially.

I searched a little but didn't see a ticket for this. is there one @BenPope?

travisdowns

I like all the code removal, but can you elaborate on why you decided to take this route over just changing the std::vector to chunked_vector?

ballard26 · 2024-03-25T21:40:43Z

I like all the code removal, but can you elaborate on why you decided to take this route over just changing the std::vector to chunked_vector?

Sure, initially I did just blindly switch it over. Then I realized chunked_vector doesn't have a delete(iterator ..) implementation. So the option was to write an implementation of that or do something else. And even if I had switched over the balancer would still be wasting a lot of time and memory book keeping information it never uses.

I implemented recommend_reassignment for the topic aware constraint with the idea that it could be used as a speed improvement over just randomly selecting reassignments and seeing if they decrease error. In testing it did prove to more quickly decrease error. However, I wasn't able to come up with a good way to combine the recommend_reassignment methods from all constraints in a way that reduced error as far as just randomly selecting and checking reassignments. A big issue with using multiple recommend_reassignment functions is they can loop. I.e, one makes a change then the other reverts it. There are ways to break the loops, but as a result error is reduced far more slowly when compared to just using one recommend_reassignment function on its own. So it never actually was put into use. I still have the data and graphs I generated during the investigation back then if you're interested.

So with that in mind recommend_reassignment is basically dead code as is. And the only thing its doing is creating more book keeping work for the balancer.

ztlpn

LGTM overall, although I wonder what the plan for removing legacy code is. Are we waiting for a specific release when we'll be able to deprecate the greedy strategy?

ztlpn · 2024-03-29T14:17:57Z

src/v/cluster/scheduling/leader_balancer_constraints.cc

-    _topic_node_index[topic_id][r.to.node_id].emplace_back(
-      moved_group_info.group_id, r.to, std::move(moved_group_info.replicas));
+    _topic_node_index.at(topic_id).at(r.from.node_id) -= 1;
+    _topic_node_index.at(topic_id).at(r.to.node_id) += 1;


I wonder why this second at is supposed to be always successful. Isn't it possible that some nodes didn't have a leader of any partition of that topic previously?

It should always be. even_topic_distributon_constraint::update_index is only used internally and the reassignment is validated by the callers before its called.

Got it, we initialize it on (new) line 79. But maybe it's easier for the reader to just use operator[]

src/v/cluster/scheduling/leader_balancer_constraints.h

…uton_constraint

ztlpn · 2024-04-04T19:29:00Z

src/v/cluster/scheduling/leader_balancer_constraints.cc

-    _topic_node_index[topic_id][r.to.node_id].emplace_back(
-      moved_group_info.group_id, r.to, std::move(moved_group_info.replicas));
+    _topic_node_index.at(topic_id).at(r.from.node_id) -= 1;
+    _topic_node_index.at(topic_id).at(r.to.node_id) += 1;


Got it, we initialize it on (new) line 79. But maybe it's easier for the reader to just use operator[]

piyushredpanda · 2024-04-05T01:34:29Z

/ci-repeat

vbotbuildovich · 2024-04-05T04:37:26Z

new failures in https://buildkite.com/redpanda/redpanda/builds/47424#018eac40-c414-4630-a9ad-be99d0604a0e:

"rptest.tests.partition_balancer_test.PartitionBalancerTest.test_unavailable_nodes"

new failures in https://buildkite.com/redpanda/redpanda/builds/48832#018f5921-de84-48a7-a7d9-1f0fb99dc2f2:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.ABS"

new failures in https://buildkite.com/redpanda/redpanda/builds/48832#018f5912-cc81-4f75-9f78-52a8fcdd2b12:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48832#018f5921-de86-4508-bf5b-6d70c6a25e71:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48832#018f5912-cc7f-4db1-b622-860beaa08233:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.ABS"

ballard26 · 2024-05-08T15:24:02Z

/ci-repeat

piyushredpanda · 2024-05-09T03:47:48Z

Failures should be fixed by #18302

vbotbuildovich · 2024-05-09T03:48:18Z

/backport v23.3.x

piyushredpanda · 2024-05-09T03:50:49Z

/backport v24.1.x

ballard26 requested review from travisdowns and StephanDollberg March 22, 2024 22:00

github-actions bot added the area/redpanda label Mar 22, 2024

travisdowns reviewed Mar 25, 2024

View reviewed changes

ballard26 requested a review from ztlpn March 25, 2024 15:36

ballard26 force-pushed the large-alloc-lb branch from ae11e01 to b031a46 Compare March 28, 2024 19:16

ballard26 requested a review from travisdowns March 28, 2024 19:17

ztlpn reviewed Mar 29, 2024

View reviewed changes

ballard26 force-pushed the large-alloc-lb branch from b031a46 to 516a874 Compare April 2, 2024 19:26

ballard26 added 2 commits April 2, 2024 15:28

cluster: remove recommended_reassignment impl from even_topic_distrib…

46e65b5

…uton_constraint

cluster: store number of leaders per node in _topic_node_index

a5be6a6

ballard26 force-pushed the large-alloc-lb branch from 516a874 to a5be6a6 Compare April 2, 2024 19:28

ztlpn approved these changes Apr 4, 2024

View reviewed changes

piyushredpanda merged commit 8c6bdf3 into redpanda-data:dev May 9, 2024
13 of 17 checks passed

This was referenced May 9, 2024

[v23.3.x] Oversized allocation: 655360 bytes in even_topic_distributon_constraint::rebuild_indexes #18325

Open

[v23.3.x] Change information stored in _topic_node_index to avoid oversized alloc #18326

Open

This was referenced May 9, 2024

[v24.1.x] Oversized allocation: 655360 bytes in even_topic_distributon_constraint::rebuild_indexes #18327

Closed

[v24.1.x] Change information stored in _topic_node_index to avoid oversized alloc #18328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change information stored in `_topic_node_index` to avoid oversized alloc #17350

Change information stored in `_topic_node_index` to avoid oversized alloc #17350

ballard26 commented Mar 22, 2024

vbotbuildovich commented Mar 23, 2024 •

edited

Loading

travisdowns Mar 25, 2024

ballard26 Mar 25, 2024

ballard26 Mar 28, 2024

BenPope Jul 19, 2024 •

edited

Loading

dotnwat Jul 27, 2024

dotnwat Jul 27, 2024

travisdowns left a comment

ballard26 commented Mar 25, 2024 •

edited

Loading

ztlpn left a comment

ztlpn Mar 29, 2024

ballard26 Apr 2, 2024

ztlpn Apr 4, 2024

ztlpn Apr 4, 2024

piyushredpanda commented Apr 5, 2024

vbotbuildovich commented Apr 5, 2024 •

edited

Loading

ballard26 commented May 8, 2024

piyushredpanda commented May 9, 2024

vbotbuildovich commented May 9, 2024

piyushredpanda commented May 9, 2024

Change information stored in _topic_node_index to avoid oversized alloc #17350

Change information stored in _topic_node_index to avoid oversized alloc #17350

Conversation

ballard26 commented Mar 22, 2024

Backports Required

Release Notes

vbotbuildovich commented Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenPope Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisdowns left a comment

Choose a reason for hiding this comment

ballard26 commented Mar 25, 2024 • edited Loading

ztlpn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyushredpanda commented Apr 5, 2024

vbotbuildovich commented Apr 5, 2024 • edited Loading

ballard26 commented May 8, 2024

piyushredpanda commented May 9, 2024

vbotbuildovich commented May 9, 2024

piyushredpanda commented May 9, 2024

Change information stored in `_topic_node_index` to avoid oversized alloc #17350

Change information stored in `_topic_node_index` to avoid oversized alloc #17350

vbotbuildovich commented Mar 23, 2024 •

edited

Loading

BenPope Jul 19, 2024 •

edited

Loading

ballard26 commented Mar 25, 2024 •

edited

Loading

vbotbuildovich commented Apr 5, 2024 •

edited

Loading