Refactor check to support different versions easily #3929
Conversation
| partition, | ||
| ) | ||
| topic_partitions_without_a_leader.append((topic, partition)) | ||
| def __init__(self, name, init_config, instances): |
There was a problem hiding this comment.
Okay, I just thought I'd have it already there for the next PR
| consumer_offsets = {} | ||
| if coord_id is not None and coord_id >= 0: | ||
| broker_ids = [coord_id] | ||
| if is_affirmative(instance.get('mode-0.10.2', False)): |
There was a problem hiding this comment.
Why not use a flag from init_config instead? That would make more sense to me, as all instances are going to be impacted.
Arguing about naming, but I think we should make it like mode_0_10_2, if we don't find a friendlier name.
There was a problem hiding this comment.
Instances may use different versions of Kafka
There was a problem hiding this comment.
Are we creating a class for every instance?
|
|
||
| return highwater_offsets, topic_partitions_without_a_leader | ||
| def __new__(cls, name, init_config, instances): | ||
| instance = instances[0] |
There was a problem hiding this comment.
Could there be a case where different instances use different modes?
If yes then the split should happen on the check method rather than the constructor, if not it should be a init config
There was a problem hiding this comment.
- Yes
- No need b/c each config instance is given its own AgentCheck instance
There was a problem hiding this comment.
But aren't we always using the first instance to check the mode?
There was a problem hiding this comment.
There is only ever one instance in that list
There was a problem hiding this comment.
Ok, we have a confusing interface for checks
There was a problem hiding this comment.
A5 was different is why :(
| consumer_offsets = {} | ||
| if coord_id is not None and coord_id >= 0: | ||
| broker_ids = [coord_id] | ||
| if is_affirmative(instance.get('mode-0.10.2', False)): |
There was a problem hiding this comment.
Maybe the issue is naming here but I'm reading this if like:
if 0.10.2:
something
else:
0.10.2
which does not make a lot of sense
| ) | ||
|
|
||
|
|
||
| class LegacyKafkaCheck_0_10_2(AgentCheck): |
There was a problem hiding this comment.
I assume this is pretty much a copypaste of old kafka_consumer.py
Codecov Report
@@ Coverage Diff @@
## master #3929 +/- ##
==========================================
- Coverage 86.31% 80.76% -5.56%
==========================================
Files 763 10 -753
Lines 40116 551 -39565
Branches 4709 114 -4595
==========================================
- Hits 34628 445 -34183
+ Misses 4196 74 -4122
+ Partials 1292 32 -1260 |
|
Did the post_0_10_2 flag ever get added? |
Additional Notes
The
post_0_10_2flag will be used in a subsequent PR