fix(gateway): store followers and inactive nodes in sets

[megglos]

Previous use of lists resulted in unbounded growth due to duplication of the same broker ids.

Description

Replaces the usage of Lists to keep track of followers and inactiveNodes in the BrokerClusterState with Sets to prevent duplication on arbitrary metadata updates.

Related issues

closes #8724

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

• The changes are backwards compatibility with previous versions
• If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

• There are unit/integration tests that verify all acceptance criterias of the issue
• New tests are written to ensure backwards compatibility with further versions
• The behavior is tested manually
• The change has been verified by a QA run
• The impact of the changes is verified by a benchmark

Documentation:

• The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
• New content is added to the release announcement
• If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

[github-actions]

Test Results

   856 files  ±  0     856 suites  ±0   1h 42m 22s ⏱️ -27s
6 704 tests  - 44  6 694 ✔️  - 44  10 💤 ±0  0 ❌ ±0 
6 888 runs   - 44  6 878 ✔️  - 44  10 💤 ±0  0 ❌ ±0 

Results for commit 649d8fa. ± Comparison against base commit fca38d2.

♻️ This comment has been updated with latest results.

[npepinpe]

👍

Fix looks good. I added one comment about tests relating to waitUntil vs Awaitility. I only added in one place, but it applies to all the places you used waitUntil.

🔧 One additional thing, as we are slowly, progressively migrating to Junit 5 (e.g. new things should be written with junit 5, assertJ, awaitility,e tc.), I would typically take the opportunity to migrate the whole test class (as a separate commit of course). I think it's perfectly fine you don't do this here as you're still just getting familiar with everything, but this is something I'd recommend in the future (when it's easy enough, of course).

Anyway, once you've addressed the Awaitility comment, I don't think we need another review again (change is fairly straightforward), but ofc feel free to request one if you'd like :)

[megglos]

🔧 One additional thing, as we are slowly, progressively migrating to Junit 5 (e.g. new things should be written with junit 5, assertJ, awaitility,e tc.), I would typically take the opportunity to migrate the whole test class (as a separate commit of course). I think it's perfectly fine you don't do this here as you're still just getting familiar with everything, but this is something I'd recommend in the future (when it's easy enough, of course).

Anyway, once you've addressed the Awaitility comment, I don't think we need another review again (change is fairly straightforward), but ofc feel free to request one if you'd like :)

Fully agree but also due to backporting labels I would leave the full junit5 migration of this class out of scope for now. Will replace all usages of waitUntil with Awaitility though, thanks for making me aware of this!

[megglos]

bors r+

[zeebe-bors-camunda]

Build failed:

• Test summary

[npepinpe]

Is this your first flaky test? 🕵️

[megglos]

bors retry

[megglos]

Is this your first flaky test? 🕵️

yes 😁 have you seen these go client tests failing before in this way?

[npepinpe]

Not the Go clients, but it can happen that the partition is initially unhealthy on startup before becoming healthy asynchronously. So it could be again this issue (just speculating). Here the PR is mostly modifying the broker cluster state (so the topology from the broker point of view) - the question is, could it have an impact on the topology from the gateway point of view? I think your PR is probably not introducing a bug there and it's a test set up issue (or you did really uncover a bug 😄), so it's probably safe to merge. In this instance, we open a new flaky test issue, and based on the test would assign it to one of the team's project.

[zeebe-bors-camunda]

Build failed:

• Test summary

[megglos]

bors retry

[zeebe-bors-camunda]

Build failed:

• Test summary

[megglos]

bors retry

[zeebe-bors-camunda]

Build succeeded:

• Java checks
• Test summary
• Go linting

[backport-action]

Successfully created backport PR #10276 for stable/1.3.

[backport-action]

Successfully created backport PR #10277 for stable/8.0.