Refactoring of topic handling in preparation of #22 #27
Conversation
MrTrustworthy
changed the title
There was a problem hiding this comment.
Since its all over the place, I'll comment it here. I'd keep the TopicController in the Cluster object. It's best to have one single object (i.e. cluster) that you can use as interface.
The TopicController should exist only once per cluster anyway and who is better to ensure this if not the cluster object itself?
The cluster object could serve as a factory for all Kafka resources so you don't have to repeat yourself.
Then you can create a cluster with some bootstrap servers (and later also credentials) and everything else can (and should) be handled from there.
| self.port = port | ||
|
|
||
| @classmethod | ||
| def from_id(cls, cluster, broker_id) -> "Broker": |
There was a problem hiding this comment.
The constructor supports the exact same signature, so what's the benefit of this method?
| return cls(cluster=cluster, broker_id=broker_id) | ||
|
|
||
| @classmethod | ||
| def from_attributes(cls, cluster, broker_id: int, host: str, port: int) -> "Broker": |
| @@ -233,7 +227,7 @@ def get_offsets(state, topic_name): | |||
| @click.argument("broker-id", required=True) | |||
| @pass_state | |||
| def describe_broker(state, broker_id): | |||
| broker = Broker(state.cluster, broker_id).describe() | |||
| broker = Broker.from_id(state.cluster, broker_id).describe() | |||
There was a problem hiding this comment.
I'd actually prefer state.cluster.get_broker(broker_id).describe()
There was a problem hiding this comment.
For the approach I'm working on, I'd use this as preparation to move this stuff to a BrokerController. See my comment at the bottom
| @@ -258,9 +252,9 @@ def describe_consumergroup(state, consumer_id, verbose): | |||
| @get.command("brokers") | |||
| @pass_state | |||
| def get_brokers(state): | |||
| brokers = state.cluster.brokers | |||
| brokers = Broker.get_all(state.cluster) | |||
There was a problem hiding this comment.
Similar to above: state.cluster.get_all_brokers() would be nicer.
By the way, brokers can have multiple endpoints. Guess by default we should use the one that esque is using. But would be good to have the possibility to list all endpoints.
I've also considered that, but decided against it in favour of the current approach. My thoughts behind that: I'd like to treat the Cluster as a "dumb" resource object similar to how the Topic should behave after this PR. The resource objects should only wrap the different clients and provide some basic utility functions. The TopicController just get's the Cluster as input, and can then create/change resources of that cluster as output. That's also how I'd structure further Controllers ex. for ConsumerGroups or Partitions. I think this approach makes the View (the CLI) a little more verbose, but should make the separation between resources and controllers more explicit and hopefully testing/mocking easier as well. But having the Cluster as god-object that manages all resources could also work, and I'm open to changing it to that approach if you think it would be better. We'll just have to agree on one architectural approach going forward. |
tests/conftest.py
Outdated
| @pytest.fixture() | ||
| def avro_producer(test_config: Config): | ||
| producer_config = test_config.create_confluent_config() | ||
| producer_config.update({"schema.registry.url": Config().schema_registry}) |
There was a problem hiding this comment.
Could it be that you want test_config.schema_registry instead of Config().schema_registry ?
There was a problem hiding this comment.
That's just in here because i merged in master :D
|
For the moment I'm okay with separating it. But personally I'd prefer a "god" object as you call it. It could be just a proxy. Imho its easier to have one object around where I can check the API using my IDE than to keep several where I first have to find and figure out how to create the one that does what I want. Looking from a usability perspective here. |
|
Some of the newer changes also close #28 by simply not showing non-editable properties in |
|
@swenzel If you want to do a follow-up PR to change it, go for it :) |
Fixes absolutely Nothing
#26