Ct testing rnot2 #27719
Ct testing rnot2 #27719
Conversation
Signed-off-by: Noah Watkins <[email protected]>
We can see from azurite debug logs that the request appears to be successfully processed and returning 201, but the redpanda client gets a 400. So it might be happening at a lower level in the Azurite server. Ticket filed with Azurite, and we may have to revisit this. Signed-off-by: Noah Watkins <[email protected]>
If we don't close the builder then we may not truncate or properly flush the file to disk.
This reverts commit 09ece07.
There was a problem hiding this comment.
Pull Request Overview
This PR adds cloud topics testing support to the random node operations test and fixes several issues in the cloud topics reconciler component. The main purpose is to enhance test coverage for cloud topics functionality and improve the robustness of the cloud topics implementation.
Key changes:
- Adds cloud topics workload to random node operations test with conditional enablement based on version compatibility
- Refactors cloud topics manager to improve leadership change handling and topic ID mapping
- Enhances resource cleanup with proper builder/reader closing and additional logging
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/rptest/tests/random_node_operations_test.py |
Adds cloud topics test workload with version-aware enablement |
src/v/cloud_topics/reconciler/reconciler.cc |
Removes cloud partition check and adds proper builder cleanup |
src/v/cloud_topics/manager/manager.h |
Adds topic ID mapping for deleted topic handling |
src/v/cloud_topics/manager/manager.cc |
Refactors leadership change logic and topic ID tracking |
src/v/cloud_topics/level_one/domain/domain_supervisor.h |
Adds leadership change notification method |
src/v/cloud_topics/level_one/domain/domain_supervisor.cc |
Replaces partition notifications with direct callback approach |
src/v/cloud_topics/level_one/domain/BUILD |
Removes unused partition change notifier dependencies |
src/v/cloud_topics/level_one/common/object.cc |
Adds destructors to ensure proper resource cleanup |
src/v/cloud_topics/housekeeper/manager.cc |
Enhances stop() method with detailed logging |
src/v/cloud_topics/app.cc |
Wires up domain supervisor leadership notifications |
| if (_partitions.contains(ntp)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The logic change removes the cloud partition check but doesn't validate that the partition is actually a cloud topic partition before attaching it. This could lead to non-cloud partitions being incorrectly processed by the reconciler.
Copilot uses AI. Check for mistakes.
| tidp, partition); | ||
| auto res = _partitions.try_emplace(ntp, std::move(attached)); | ||
| vassert(res.second, "Double registration of ntp {}", ntp); | ||
| _partitions.emplace(ntp, std::move(attached)); |
There was a problem hiding this comment.
Replacing try_emplace with emplace removes the duplicate registration protection. If _partitions.contains(ntp) check above fails to prevent duplicates, this could overwrite existing partition entries without warning.
| _partitions.emplace(ntp, std::move(attached)); | |
| _partitions.try_emplace(ntp, std::move(attached)); |
Copilot uses AI. Check for mistakes.
| ~object_builder_impl() override { | ||
| vassert(_closed, "L1 object builders must be closed unconditionally"); | ||
| } |
There was a problem hiding this comment.
Using vassert in a destructor can cause program termination during stack unwinding if an exception is already being processed. Consider using a different error handling mechanism like logging an error instead.
Copilot uses AI. Check for mistakes.
| : _input(std::move(input)) {} | ||
|
|
||
| ~object_reader_impl() override { | ||
| vassert(_closed, "L1 object readers must be closed unconditionally"); |
There was a problem hiding this comment.
Using vassert in a destructor can cause program termination during stack unwinding if an exception is already being processed. Consider using a different error handling mechanism like logging an error instead.
| vassert(_closed, "L1 object readers must be closed unconditionally"); | |
| if (!_closed) { | |
| fmt::print(stderr, "ERROR: L1 object readers must be closed unconditionally\n"); | |
| } |
Copilot uses AI. Check for mistakes.
Retry command for Build#72891please wait until all jobs are finished before running the slash command |
Backports Required
Release Notes