Repository navigation
Conversation
| indoc = "2.0" | ||
| kafka-protocol = { version = "0.18", default-features = false, features = ["broker", "client"] } | ||
| rstest = "0.26" | ||
| rustls = { version = "0.23", default-features = false, features = ["logging", "ring", "std", "tls12"] } |
There was a problem hiding this comment.
Note to self, check if we enable the LogTracer (to get the logs). In 0.24, rustls will enable tracing so we won't need a log bridge for that.
| if self.host.contains(':') { | ||
| write!(f, "[{}]:{}", self.host, self.port) | ||
| } else { | ||
| write!(f, "{}:{}", self.host, self.port) | ||
| } |
There was a problem hiding this comment.
nit: We should avoid positional (unnamed) interpolations.
Something like this reads nicer:
| if self.host.contains(':') { | |
| write!(f, "[{}]:{}", self.host, self.port) | |
| } else { | |
| write!(f, "{}:{}", self.host, self.port) | |
| } | |
| let Self { host, port } = self; | |
| if self.host.contains(':') { | |
| write!(f, "[{host}]:{port}") | |
| } else { | |
| write!(f, "{host}:{port}") | |
| } |
There was a problem hiding this comment.
To be even more sure, we could check by parsing the ipv6 address instead of looking for :.
| describe_topic_configs_result(response, topic) | ||
| } | ||
|
|
||
| /// `None` uses the broker default for the number of partitions or the replication factor. |
There was a problem hiding this comment.
This comment is unclear what it refers to. It should describe the function?
There was a problem hiding this comment.
Improved in b00fe65
(The comment was referring to the functions behaviour when the optional replication factor and partitions are None)
| .context(MissingResultSnafu { api_key, topic }) | ||
| } | ||
|
|
||
| fn check(api_key: i16, topic: &str, error_code: i16, message: Option<StrBytes>) -> Result<()> { |
There was a problem hiding this comment.
Would prefer better naming, what gets checked?
| } | ||
|
|
||
| // Used instead of [0] to correctly handle empty results & take ownership | ||
| fn first<T>(results: Vec<T>, api_key: i16, topic: &str) -> Result<T> { |
There was a problem hiding this comment.
Here also, would prefer better naming, I assume it something like return_first_result?
| // Without the details, e.g. if `DESCRIBE_CONFIGS` is not allowed, the topic was still created. | ||
| check( | ||
| CreateTopicsRequest::KEY, | ||
| topic, | ||
| result.topic_config_error_code, | ||
| None, | ||
| )?; |
There was a problem hiding this comment.
I don't understand this part. So the topic gets created but the user still sees an error but also without an error message? So how would they understand what went wrong while the topic is still there now?
There was a problem hiding this comment.
The error is surfaced to the user in the KafkaTopic condition as KafkaRejected with a message like the broker rejected the CreateTopics request for the topic "<topic>" with TopicAuthorizationFailed. We check the response from Kafka for an error code which then gets returned to the caller as the Error::Kafka variant.
| const METADATA_VERSION: i16 = 12; | ||
| const CREATE_TOPICS_VERSION: i16 = 7; | ||
| const DELETE_TOPICS_VERSION: i16 = 6; | ||
| const CREATE_PARTITIONS_VERSION: i16 = 3; | ||
| const DESCRIBE_CONFIGS_VERSION: i16 = 4; | ||
| const INCREMENTAL_ALTER_CONFIGS_VERSION: i16 = 1; | ||
| const LIST_PARTITION_REASSIGNMENTS_VERSION: i16 = 0; | ||
|
|
||
| /// Must stay below the connection's request timeout. | ||
| const BROKER_TIMEOUT_MS: i32 = 25_000; | ||
| const _: () = assert!((BROKER_TIMEOUT_MS as u128) < connection::REQUEST_TIMEOUT.as_millis()); | ||
|
|
||
| /// Must stay below the broker's `connections.max.idle.ms` (10 minutes by default). | ||
| const CONNECTION_MAX_IDLE: Duration = Duration::from_secs(5 * 60); | ||
|
|
||
| /// Lets the broker apply `num.partitions` or `default.replication.factor` when creating a topic. | ||
| const BROKER_DEFAULT: i32 = -1; | ||
|
|
||
| const RESOURCE_TYPE_TOPIC: i8 = 2; | ||
| const CONFIG_OPERATION_SET: i8 = 0; | ||
| const CONFIG_OPERATION_DELETE: i8 = 1; |
There was a problem hiding this comment.
I don't understand all those numbers here, could you add some documentation to that please? Why and how were those numbers chosen? If someone needs to maintain this they need some guidance I would say.
There was a problem hiding this comment.
Added documentation on the source for the constants in 56b9894.
Part of stackabletech/issues#888.
Stacked on #1028.
Description
stackable-kafka-adminwith the requests for describing, creating and deleting topics, adding partitions, reading and changing topic configs, and listing partition reassignments.3.9.2and4.3.1both support and checked against the versions the broker advertises.Out-of-scope
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker