Skip to content

KAFKA-20645: Move LogLoaderTest to storage module#22426

Open
mimaison wants to merge 2 commits into
apache:trunkfrom
mimaison:kafka-20645
Open

KAFKA-20645: Move LogLoaderTest to storage module#22426
mimaison wants to merge 2 commits into
apache:trunkfrom
mimaison:kafka-20645

Conversation

@mimaison
Copy link
Copy Markdown
Member

@mimaison mimaison commented May 30, 2026

Also delete LogTestUtils.scala as it's not used anymore

Reviewers: Ken Huang s7133700@gmail.com

@github-actions github-actions Bot added core Kafka Broker storage Pull requests that target the storage module labels May 30, 2026
LogManager logManager = interceptedLogManager(logConfig, logDirs, logDirFailureChannel, time,
cleanShutdownInterceptedValue, simulateError);
logManager.getOrCreateLog(topicPartition, true, false, Optional.empty());
assertFalse(logDirFailureChannel.hasOfflineLogDir(logDir.getAbsolutePath()), "log dir should not be offline before load logs");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch!

It looks like this is now using the class-level logDir, whereas the Scala version used the logDir created inside testLogRecoveryIsCalledUponBrokerCrash.

This means the assertion may check a different directory after the migration. Should this use the local logDir instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nice find! This is indeed a mistake, I pushed a fix. Thanks

Copy link
Copy Markdown
Collaborator

@m1a2st m1a2st left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Kafka Broker storage Pull requests that target the storage module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants