Remove unnecessary methods from TopicOperatorUtil class #10918
Conversation
topic-operator/src/main/java/io/strimzi/operator/topic/TopicOperatorMain.java
Outdated
Show resolved
Hide resolved
| public static CruiseControlClient.Config mapToCcClientCfg(TopicOperatorConfig operatorCfg) { | ||
| return new CruiseControlClient.Config( | ||
| operatorCfg.cruiseControlHostname(), | ||
| operatorCfg.cruiseControlPort(), | ||
| operatorCfg.cruiseControlRackEnabled(), | ||
| operatorCfg.cruiseControlSslEnabled(), | ||
| operatorCfg.cruiseControlSslEnabled() ? getFileContent(operatorCfg.cruiseControlCrtFilePath()) : null, | ||
| operatorCfg.cruiseControlAuthEnabled(), | ||
| operatorCfg.cruiseControlAuthEnabled() | ||
| ? new String(getFileContent(operatorCfg.cruiseControlApiUserPath()), StandardCharsets.UTF_8) : null, | ||
| operatorCfg.cruiseControlAuthEnabled() | ||
| ? new String(getFileContent(operatorCfg.cruiseControlApiPassPath()), StandardCharsets.UTF_8) : null |
There was a problem hiding this comment.
What exactly is the point of this change? It seems like you didn't really improved anything just made it more complicated.
The methods for the Kubernetes client or Admin client are just forwarding the call somewhere else without doing anything. But this seems to have some logic and save some code / improve readability. So maybe this should be kept as it was?
There was a problem hiding this comment.
It's a simple config mapping. The improvement here is in the CC client API. In terms of util method, it doesn't change much.
There was a problem hiding this comment.
We trade one method for two methods and a class. That does not seem like a simplification to me.
There was a problem hiding this comment.
The CC client "create" method now takes a config object as input, like the other two clients (Kube and Admin). This is the improvement.
There was a problem hiding this comment.
Sorry, but ... for me it just creates yet another unnecessary class (record) that is essentially used in a single place.
There was a problem hiding this comment.
my 2 cents ...
In general, I don't like having a class/record with so many parameters and AFAICS the current TopicOperatorConfig record has 33 parameters. I like the idea of grouping things logically. I am not sure it's going to increase memory usage or complexity tbh, but it would increase readability imho.
On the other side, it's also true that the change could be done when we are moving out the Cruise Control logic from the TO in order to have a shared client but I can't see these changes hurting here right now. Said that I can see both what Jakub said but as well as what Fede said so I am not against these changes.
I also don't think that Fede is ignoring comments, he just replied here #10918 (comment). Reviewing a PR doesn't mean that the author should include all the suggestions provided to him. This is why, it's important having a broader pool of people to review a PR in order to avoid a 1 to 1 which doesn't go in any direction :-).
I have just a question ... do we still need the TopicOperatorConfig record having 33 parameters? I see it's still in place because used within the other ctor getting the map.
There was a problem hiding this comment.
I have just a question ... do we still need the TopicOperatorConfig record having 33 parameters? I see it's still in place because used within the other ctor getting the map.
That's a good point. TBH, I have no idea why this was done differently from the other operators. Let me see if I can fix it in this PR without too much caos, otherwise I'll create a separate PR. Thanks.
There was a problem hiding this comment.
@fvaleri to be precise, my question above was strictly related to this PR. I was wondering if we still need that so huge constructor if moving to use a different record for CC configuration. About not having the 33 parameters in the main codebase I don't know.
There was a problem hiding this comment.
We still need 33 config keys to avoid breaking the standalone user interface, but most of them are optional. That said, we can simply have one constructor that takes a config map, like we have in all the other operators. I will open a dedicated PR so that we can discuss separately on this.
There was a problem hiding this comment.
Let's wait for that PR being merged so that you can rebase this one.
topic-operator/src/main/java/io/strimzi/operator/topic/TopicOperatorMain.java
Outdated
Show resolved
Hide resolved
fvaleri
force-pushed
the
to-utils
branch
2 times, most recently
from
7054e5d to
c1b176f
Compare
This change makes the TopicOperatorConfig class consistent with the other operators. For more info see: strimzi#10918 (comment) Signed-off-by: Federico Valeri <[email protected]>
This change makes the TopicOperatorConfig class consistent with the other operators. For more info see: strimzi#10918 (comment) Signed-off-by: Federico Valeri <[email protected]>
This change makes the TopicOperatorConfig class consistent with the other operators. For more info see: strimzi#10918 (comment) Signed-off-by: Federico Valeri <[email protected]>
This change makes the TopicOperatorConfig class consistent with the other operators. For more info see: strimzi#10918 (comment) Signed-off-by: Federico Valeri <[email protected]>
This patch removes unnecessary methods from TopicOperatorUtil class. Closes strimzi#10916. Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
There was a problem hiding this comment.
I added a comment around the new Config record, but I also think this PR needs to be renamed as "Remove unnecessary methods from TopicOperatorUtil class" seems misleading to me. I think something like "Refactor TopicOperatorConfig initialisation" would better describe the changes.
| * @param authUsername Authentication username. | ||
| * @param authPassword Authentication password. | ||
| */ | ||
| record Config( |
There was a problem hiding this comment.
I do think it's useful to encapsulate the configuration so it can be passed around a single variable, however we still have to initialise this with 8 parameters in the specific order. Would it be better to either go the whole way and make this a full class which is Buildable to make it nicer to initialise it, or if sticking with the record at least include the validation here as a custom constructor? That way we are getting added value from the new object.
There was a problem hiding this comment.
Good point. I am fine with both. We use the builder pattern across the project in other places as well (i.e. broker configuration).
There was a problem hiding this comment.
Will you replace every method that takes 4 parameters and is called from a single place with a Config Builder?
There was a problem hiding this comment.
I'm not suggesting we follow this pattern everywhere. What I meant is I can understand why this change has been suggested, however if we are going to add an additional object (whether record or class) I think in order for it to be worth the overhead it should be more than just a holder for some parameters, since as you said it's only called in a single place.
There was a problem hiding this comment.
Thanks for the review. Appreciated. As you suggested, I'm moving the input validation to the Config object constructor, as it is something I already explored in a previous commit, and I agree that it adds more value.
Signed-off-by: Federico Valeri <[email protected]>
Type of change
Description
This patch removes unnecessary methods from
TopicOperatorUtilclass.Like the Kafka admin and Kubernetes clients, the CC client was designed as a general purpose client that can potentially be reused. As part of this change, I'm also adding CC client input validation and making its creation consistent with the other clients.
New tests are also added for the TO's configuration class.
Closes #10916.
Checklist