confluent-kafka-dotnet
confluent-kafka-dotnet copied to clipboard
confluentinc
Example of Assign() is slightly misleading
Description
While consumer group is not used in Assign() scenarios, it still needs to be provided for whatever reason. In the samples new Guid() is used for naming that group. It seems to be misleading, because in most scenarios Guids are used as unique identifiers, but in this case it is just a constant (00000000-0000-0000-0000-000000000000).
As a result developers often get incorrect assumption that consumer group names for Assign() should be unique (even with each service restart). They just blindly miss that Guid is created via new Guid() but not Guid.NewGuid(). And it ends up with hundreds of empty consumer groups with Guid names in clusters.
I seems to be better to specify some hardcoded constant (and probably explicitly state that that value could be used for ALL services if needed).
How to reproduce
- Change this line to
GroupId = Guid.NewGuid().ToString(), - Run sample few times
- New consumer group with unique name is created each time. Old groups are not removed.
new Guid()
Perhaps Guid.Empty might be a better way to convey the fact that the value should remain stable?
Though I'd favor "TestConsumerGroupName" or something more explicit
yeah, that's probably better. we could probably improve some of the comment text as well.
https://github.com/confluentinc/confluent-kafka-dotnet/pull/1914
except I didn't do:
seems to be better to specify some hardcoded constant (should)
also, there are some comments that don't line up with the code in this example.
also we should demonstrate store offset, as this is best practice.
#1914 new Guid() gives 00000000-0000-0000-0000-000000000000, Guid.NewGuid() gives a new unique guid, which is the intent.
I think the intent is the opposite. Guid.NewGuid() creates a new consumer group during every service restart, cluttering the cluster.
These groups are not used in assign scenario. Only one group with hardened id should be enough for all consumers.
