KAFKA-17689: Remove unused TieredStorageTestHarness#22445
Open
joshua2519 wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
triage
Russole
reviewed
Jun 3, 2026
Russole
left a comment
Russole
left a comment
There was a problem hiding this comment.
Thanks @joshua2519 for working on this. Mostly LGTM. Just left a few minor comments.
| # The Test Flow | ||
|
|
||
| Step 1: For every test, setup is done via TieredStorageTestHarness which extends IntegrationTestHarness and sets up a cluster with TS enabled on it. | ||
| Step 1: Each test is a standalone class. It declares a `clusterConfig()` method that returns a `ClusterConfig` with tiered storage enabled (via `TieredStorageTestUtils.createServerPropsForRemoteStorage`), and test methods annotated with `@ClusterTemplate("clusterConfig")` that receive a `ClusterInstance` provided by the test framework. |
There was a problem hiding this comment.
nit: The current wording says clusterConfig() returns a ClusterConfig, but the tiered storage tests generally return List<ClusterConfig> from their @ClusterTemplate methods. Could we make this slightly more precise, e.g. “returns one or more ClusterConfig instances, typically as List<ClusterConfig>”?
m1a2st
reviewed
Jun 6, 2026
Collaborator
m1a2st
left a comment
m1a2st
left a comment
There was a problem hiding this comment.
Thanks @joshua2519 for this patch, two minor comments
| private static final Integer RLM_TASK_INTERVAL_MS = 500; | ||
| private static final Integer RLMM_INIT_RETRY_INTERVAL_MS = 300; | ||
|
|
||
| public static List<LocalTieredStorage> remoteStorageManagers(Collection<KafkaBroker> brokers) { |
Collaborator
There was a problem hiding this comment.
Since only TieredStorageTestContext uses this method, we can move it there.
| return storages; | ||
| } | ||
|
|
||
| public static List<BrokerLocalStorage> localStorages(Collection<KafkaBroker> brokers) { |
chia7712
reviewed
Jun 6, 2026
| return brokers.stream() | ||
| .map(b -> new BrokerLocalStorage(b.config().brokerId(), Set.copyOf(b.config().logDirs()), | ||
| STORAGE_WAIT_TIMEOUT_SEC)) | ||
| .collect(Collectors.toList()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nothing extends
TieredStorageTestHarnesssince the tiered-storage integration tests moved to the@ClusterTemplate+ClusterInstancepattern. This deletes it and its now-dead adapterHarnessBackedClusterInstance, moves the two still-used static helpers (remoteStorageManagers,localStorages) intoTieredStorageTestUtils, and refreshes the README. Test-only change.