Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (35.29%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #4249 +/- ##
=============================================
- Coverage 87.54% 70.18% -17.37%
Complexity 1575 1575
=============================================
Files 1283 1281 -2
Lines 223353 187888 -35465
Branches 186716 151251 -35465
=============================================
- Hits 195537 131868 -63669
- Misses 23106 51188 +28082
- Partials 4710 4832 +122
🚀 New features to boost your workflow:
|
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
findings on lines outside the diff:
warning: core/connectors/runtime/src/source.rs:508 and :513 still parse the names numeric-first with Identifier::try_from, while IggyClient::producer now uses Identifier::named, so a digit-only name makes the durability gate check one topic and the producer write to another. switch both to Identifier::named.
nit: client.producer("1", "2") used to address stream id 1 and now creates a stream named "1". a short migration note in core/sdk/README.md next to the IggyConsumerConfig note would help, the crate has no changelog.
nit: item 2 under "What changed?" in the description stops at "Also remove".
pre-existing, not blocking:
warning: IggyProducerBuilder::stream() and topic() (core/sdk/src/clients/producer_builder.rs:90) accept any Identifier while the config keeps the names, so .stream(Identifier::numeric(7)?) makes init() check stream 7 and then create a stream named after the original string. drop the setters or reject a mismatch.
warning: the HTTP handlers parse path segments with Identifier::from_str_value (core/server/src/http/handlers.rs:334, :373, :939 and more), so over HTTP a digit-only stream name still resolves as a numeric id. this fix holds for the binary transports only, worth a note.
nit: examples/rust/src/shared/system.rs:76 parses --stream-id numeric-first, creates the stream by name at line 84, then creates the topic under the parsed id. Identifier::named fixes it.
| @@ -722,8 +723,8 @@ impl IggyClient { | |||
| self.client.clone(), | |||
| name.to_owned(), | |||
| Consumer::group(name.try_into()?), | |||
There was a problem hiding this comment.
warning: this still parses a digit-only group name as a numeric id, but the join at consumer.rs:1437 now goes by name, so a member joins one address and polls, syncs and leaves by another. use Identifier::named(name)? here too.
| use iggy::stream_builder::{IggyConsumerConfig, IggyStreamConsumer}; | ||
| use integration::iggy_harness; | ||
|
|
||
| const STREAM_NAME: &str = "stream-builder-stream"; |
There was a problem hiding this comment.
warning: both names are non-numeric, so the old numeric-first parsing and Identifier::named give the same identifier and this passes before and after the fix. add a case with a digit-only name like "42" (not "0", ids start at 0).
| ) -> Result<Self, IggyError> { | ||
| let stream_id = Identifier::from_str_value(stream)?; | ||
| let topic_id = Identifier::from_str_value(topic)?; | ||
| Identifier::named(stream)?; |
There was a problem hiding this comment.
nit: the result is dropped, so this only rejects an empty or over-long name. either say so in a comment or drop both calls and the Identifier import, the build path runs the same check anyway.
also at config_iggy_producer.rs:127.
| @@ -43,13 +43,14 @@ pub(crate) async fn build_iggy_stream_topic_if_not_exists( | |||
| client: &IggyClient, | |||
| config: &IggyConsumerConfig, | |||
There was a problem hiding this comment.
nit: the doc above still says IggyProducerConfig (lines 28 and 36) but the parameter is IggyConsumerConfig. line 31 also doubles "and return" and ends in a stray "d".
| }; | ||
|
|
||
| let consumer_group_id = name.to_owned().try_into()?; | ||
| let consumer_group_id = Identifier::named(&name)?; |
There was a problem hiding this comment.
simplification: _id from the match above is never read, and every caller passes the same string as name and id, so let name = consumer_name; does the job. drop the match and the get_u32_value()?.
| pub fn topic_name(&self) -> &str { | ||
| self.producer_config.topic_name() | ||
| } | ||
| } |
There was a problem hiding this comment.
simplification: this second impl IggyStreamConfig block now holds only two getters. fold them into the block above.
github-actions
Bot
added
S-waiting-on-author
2 participants
Which issue does this PR address?
Closes #4155
Rationale
User sets both
topic_name,topic_idandstream_nameandstream_id.Both are required to align internally. If they are not, the API checks if the topic exists (and also creates if not) based on the name but binds using the topic_id.
What changed?
topic_idandstream_idfrom the public API. Rather, generate those id's always from names usingIdentifier::named().Local Execution
AI Usage
Integration test written by Claude.
/small-team-review ran on changes.