KAFKA-15633: Fix overwrite of meta.properties at startup to handle JBOD.#14578
KAFKA-15633: Fix overwrite of meta.properties at startup to handle JBOD.#14578pprovenzano wants to merge 5 commits intoapache:trunkfrom
Conversation
… directory.id. Fix reading of the meta.properties file to ignore the directory.id when validating that all the meta.properties files belong to the same cluster.
We create the file and in this case we MUST create a random Uuid for it.
|
Adding of the directory.id in any form will prevent the cluster from rolling back to a previous AK version. |
This actually is only an issue for people running JBOD. Those with only one directory per broker will be fine. |
soarez
left a comment
soarez
left a comment
There was a problem hiding this comment.
I think we should add a test for this. Perhaps we can add or extend one of the existing integration tests?
| try { | ||
| brokerCheckpoint.read() match { | ||
| case Some(properties) => | ||
| // XXX Should we check for duplicates here |
There was a problem hiding this comment.
Is LogManager.directoryIds() a better place to check for duplicates?
There was a problem hiding this comment.
I've added it there. Let me know what you think
| val props = brokerMetadata.toProperties.clone().asInstanceOf[Properties] | ||
| // If the Uuid is not set then we set it here because originally we ignored | ||
| // directories with no meta.properties file and we are creating it here. | ||
| props.setProperty("directory.id", logManager.directoryId(logDir).getOrElse(Uuid.randomUuid()).toString) |
There was a problem hiding this comment.
LogManager is already generating a directory ID if missing, in directoryIds.
There was a problem hiding this comment.
Okay, I fixed LogManager to generate the directory ID for all cases where it is missing. This includes if the meta.properties is missing which can only occur if we are in ZK mde. It does not create the missing meta.properties file until after the broker is registered with ZK.
pprovenzano
changed the title
I'd like to do that as a separate PR. |
| if (s.contains(uuid)) { | ||
| throw new RuntimeException(s"Found duplicate directory.ids ${uuid.toString}") | ||
| } |
There was a problem hiding this comment.
An alternative to maintaining a mutable set and modifying it from within the flatMap lamda could be to compare the size of the result with .toSet.size or .toSeq.distinct.size e.g.
if (dirIds.values.toSet.size != dirIds.values.size)
throw new RuntimeException(s"Found duplicate directory.ids ${uuid.toString}")There was a problem hiding this comment.
Perhaps KafkaException is more suitable than RuntimeException?
| case None => { | ||
| Uuid.randomUuid() |
There was a problem hiding this comment.
I'm not sure about this, maybe I'm missing something. So if meta.properties does not exist for some directory, we generate a random one, but it seems the generated value isn't persisted? That means the same log directory will get a different ID ever time the broker restarts, which will generate a lot of unnecessary reassignments...
There was a problem hiding this comment.
It will get persisted. The only case where we allow a directory to not have a meta.properties file is in Zk mode. In Zk mode on startup, all directories without a meta.properties file have one created immediately after the broker is registered with ZooKeeper.
|
Closing this PR in favor of #14628 |
3 participants
Fix startup to write back the updated meta.properties file with the new directory.id.
Fix reading of the meta.properties file to ignore the directory.id when validating that all the meta.properties files belong to the same cluster.