MINOR: MetaProperties refactor, part 1#14678
Merged
cmccabe merged 1 commit intoapache:trunkfrom Nov 2, 2023
Merged
Conversation
Since we have added directory.id to MetaProperties, it is no longer safe to assume that all directories on a node contain the same MetaProperties. Therefore, we should get rid of places where we are using a single MetaProperties object to represent the settings of an entire cluster. This PR removes a few such cases. In each case, it is sufficient just to pass cluster ID. The second part of this change refactors KafkaClusterTestKit so that we convert paths to absolute before creating BrokerNode and ControllerNode objects, rather than after. This prepares the way for storing an ensemble of MetaProperties objects in BrokerNode and ControllerNode, which we will do in a follow-up change.
cmccabe
force-pushed
the
remove_useless_uses_of_metaprops
branch
from
1ac9fd6 to
114f92f
Compare
cmccabe
changed the title
rondagostino
reviewed
Nov 2, 2023
Contributor
rondagostino
left a comment
rondagostino
left a comment
There was a problem hiding this comment.
LGTM in general, just left a couple of nits/small questions. Could potentially merge as-is, but let me know what you think.
The JDK 11 and JDK 17 tests were clean!
| bootstrapMetadataVersion, | ||
| controllerNodes, | ||
| brokerNodes); | ||
| } catch (Exception e) { |
Contributor
There was a problem hiding this comment.
nit: can be catch (RuntimeException e) { since the code doesn't throw any checked exceptions and the signature of this method does not declare any checked exceptions as being potentially thrown.
Contributor
Author
There was a problem hiding this comment.
Yeah, that's fair. We can only have RuntimException here. Still, I can see no logical reason why we wouldn't do the deletion if the code was restructured to throw checked exceptions here. So it seems clearer this way.
Comment on lines
-91
to
+86
| metaProperties, | ||
| Uuid.ZERO_UUID.toString, |
Contributor
There was a problem hiding this comment.
Do we want Uuid.randomUuid.toString instead, which is what we use in RaftManagerTest?
Contributor
Author
There was a problem hiding this comment.
It's a good question. I don't know why this test chose Uuid.ZERO_UUID. But I figured changing it here was out of scope.
yyu1993
pushed a commit
to yyu1993/kafka
that referenced
this pull request
Feb 15, 2024
Since we have added directory.id to MetaProperties, it is no longer safe to assume that all directories on a node contain the same MetaProperties. Therefore, we should get rid of places where we are using a single MetaProperties object to represent the settings of an entire cluster. This PR removes a few such cases. In each case, it is sufficient just to pass cluster ID. The second part of this change refactors KafkaClusterTestKit so that we convert paths to absolute before creating BrokerNode and ControllerNode objects, rather than after. This prepares the way for storing an ensemble of MetaProperties objects in BrokerNode and ControllerNode, which we will do in a follow-up change. Reviewers: Ron Dagostino <rndgstn@gmail.com>
AnatolyPopov
pushed a commit
to aiven/kafka
that referenced
this pull request
Feb 16, 2024
Since we have added directory.id to MetaProperties, it is no longer safe to assume that all directories on a node contain the same MetaProperties. Therefore, we should get rid of places where we are using a single MetaProperties object to represent the settings of an entire cluster. This PR removes a few such cases. In each case, it is sufficient just to pass cluster ID. The second part of this change refactors KafkaClusterTestKit so that we convert paths to absolute before creating BrokerNode and ControllerNode objects, rather than after. This prepares the way for storing an ensemble of MetaProperties objects in BrokerNode and ControllerNode, which we will do in a follow-up change. Reviewers: Ron Dagostino <rndgstn@gmail.com>
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
2 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.
Since we have added directory.id to MetaProperties, it is no longer safe
to assume that all directories on a node contain the same MetaProperties.
Therefore, we should get rid of places where we are using a single
MetaProperties object to represent the settings of an entire cluster.
This PR removes a few such cases. In each case, it is sufficient just to
pass cluster ID.
The second part of this change refactors KafkaClusterTestKit so that we
convert paths to absolute before creating BrokerNode and ControllerNode
objects, rather than after. This prepares the way for storing an
ensemble of MetaProperties objects in BrokerNode and ControllerNode,
which we will do in a follow-up change.