kevin-wu24 commented on code in PR #20707:
URL: https://github.com/apache/kafka/pull/20707#discussion_r2577658203
##########
metadata/src/test/java/org/apache/kafka/metadata/storage/FormatterTest.java:
##########
@@ -168,9 +183,9 @@ public void testFormatterFailsOnUnwritableDirectory()
throws Exception {
try (TestEnv testEnv = new TestEnv(1)) {
new File(testEnv.directory(0)).setReadOnly();
FormatterContext formatter1 = testEnv.newFormatter();
- String expectedPrefix = "Error while writing meta.properties file";
+ String expectedPrefix = "Error creating temporary file, logDir =";
assertEquals(expectedPrefix,
- assertThrows(FormatterException.class,
+ assertThrows(UncheckedIOException.class,
Review Comment:
Let's investigate why this error message changed. Ideally, our changes
shouldn't impact this code path, and we should still be throwing a
`FormatterException`.
##########
metadata/src/main/java/org/apache/kafka/metadata/bootstrap/BootstrapDirectory.java:
##########
@@ -55,7 +59,7 @@ public BootstrapDirectory(
this.directoryPath = Objects.requireNonNull(directoryPath);
}
- public BootstrapMetadata read() throws Exception {
+ public BootstrapMetadata maybeReadLegacyBootstrapCheckpoint() throws
Exception {
Review Comment:
I think a cleaner approach might be to make `BootstrapDirectory` an
interface with just the `read()` method. Then we have an implementation called
`LegacyBootstrapDirectory` that we use in actual Kafka that points to the
`bootstrap.checkpoint` file which is read at the `KafkaRaftServer` level. The
test code that initializes a `BootstrapDirectory` can have a test-only
implementation of the interface that points to the `0-0.checkpoint` file to
read it in memory without needing to set up KRaft. This way we do not need two
separate `read()` methods in the same class.
Let me know what you think.
##########
test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/KafkaClusterTestKit.java:
##########
@@ -482,6 +486,23 @@ private void formatNode(
formatter.setUnstableFeatureVersionsEnabled(true);
formatter.setIgnoreFormatted(false);
formatter.setControllerListenerName(controllerListenerName);
+
+ for (Feature feature : Feature.PRODUCTION_FEATURES) {
+ String featureName = feature.featureName();
+ if (featureName.equals(MetadataVersion.FEATURE_NAME) ||
+ featureName.equals(KRaftVersion.FEATURE_NAME)) {
+ continue;
+ }
+ short level =
nodes.bootstrapMetadata().featureLevel(featureName);
+ formatter.setFeatureLevel(featureName, level);
+ }
+ List<ApiMessageAndVersion> configRecords =
nodes.bootstrapMetadata().records().stream()
+ .filter(record -> record.message() instanceof ConfigRecord)
+ .collect(Collectors.toList());
+ if (!configRecords.isEmpty()) {
+ formatter.setAdditionalBootstrapRecords(configRecords);
+ }
Review Comment:
We added this to pass
`KRaftClusterTest#testStartupWithNonDefaultKControllerDynamicConfiguration()`
right? The formatter (which is how `bootstrap.checkpoint` is generated) has
never been able to write arbitrary config records, so I don't think we should
keep that test.
I don't think that test above makes sense in that file (essentially what it
is testing is that dynamic config updates are reflected in the controller
state). We can add a test in `DynamicBrokerReconfigurationTest` for this
behavior, instead of adding `Formatter` functionality that is only used in
testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
