Updated BrokerMetadata#nodeId type from int to int | str.

[jackgene]

Changes

Fixes #1050

Preferred (less invasive) fix to issue above, changes the type of aiokafka.structs.BrokerMetadata#nodeId from int to int | str. No functionality change.

Use this or #1052, but not both.

Checklist

The check list mentions a CHANGES folder, but I do not see one, am I supposed to create one for each PR?

[ods]

Hi @jackgene, thank you for the contribution. You're right, the the type annotation doesn't match the code. But I'm not sure we need to fix the type annotation here and not the code.

[jackgene]

Hi @jackgene, thank you for the contribution. You're right, the the type annotation doesn't match the code. But I'm not sure we need to fix the type annotation here and not the code.

Yup, and I did offer two PRs, though they both touch the type annotation.

As I understand it, in the aiokafka model, node_id is used for two things:

• int ID, as assigned by Kafka
• Synthetically generated str ID to denote broker/coordinator by the aiokafka library

We obviously do not want the two to collide, and it won't in the current implementation, as one is an int and the other is a str. We can keep things the way they are, and make it explicit that the type of node_id is int | str.

Alternatively, we can make it a single type, str (in my other PR), makes it easy to distinguish between the two classes of ids (numeric strings, vs strings prefixed by broker-/coordinator-).

We could also make it an int, but this require knowledge of how Kafka assigns IDs that I'm unfortunately not familiar with. If Kafka only assigns positive int for instance, we could use negative ints to represent broker/coordinators.

So IMO, changing the type annotation is the less invasive option.

[ods]

Ok, as a temporary solution it's reasonable to keep it as-is. Let's go this way. Please rebase it and add a FIXME comment explaining why it's defined this way

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.10%. Comparing base (23efe72) to head (db572e3).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
+ Coverage   95.08%   95.10%   +0.01%     
==========================================
  Files         114      114              
  Lines       16980    16981       +1     
  Branches     1579     1579              
==========================================
+ Hits        16145    16149       +4     
+ Misses        486      483       -3     
  Partials      349      349              

Flag	Coverage Δ
cext	94.90% <100.00%> (+0.01%)	⬆️
integration	94.83% <100.00%> (+0.01%)	⬆️
purepy	94.90% <100.00%> (+0.01%)	⬆️
unit	53.08% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.