[
https://issues.apache.org/jira/browse/KAFKA-5676?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17354118#comment-17354118
]
Marco Lotz edited comment on KAFKA-5676 at 5/30/21, 10:47 PM:
--------------------------------------------------------------
[~mjsax] [~cadonna] I have spent a reasonable amount of time analysing this
ticket. It seems to me that this is tightly coupled in many points (which also
explains the fair amount of unmerged PRs related to it). Based on the previous
comments, I found out the following in the code:
* As [~chia7712] mentioned, if we keep the MockStreamsMetrics class and make
it implement the interface, we would need to extend the interface (thus a KIP).
Specially because the MockStreamsMetrics is used interchangeably with
StreamsMetricsImpl class. The implementation has tons of extra public methods.
Including static ones. There is a huge amount of test classes (20+) counting on
StreamsMetricsImpl behaviours provided by MockStreamsMetrics.
* If we go for removing MockStreamsMetrics and trying to mock
StreamsMetricsImpl whenever it’s not truly needed:
# One will find out that many methods in the class are final (which is not a
limitation since PowerMock is available in Kafka project). But is a sign that
it may require further design attention. Among them is
{code:java}
public final Sensor taskLevelSensor(...){code}
# One will uncover that many static final methods are also called for
StreamsMetricsImpl (that can also be solved by PowerMock). Among them is:
{code:java}
public static void StreamsMetricsImpl.addValueMetricToSensor(...){code}
# One will find out that many test classes (e.g. StreamTaskTest or
RecordCollectorTest) would require a considerable amount of behaviour mocking.
Seems easily 20+ test classes would need considerable amount of mock
configuration in this scenario. A possible solution is indeed making a mocking
utils to init mocks of StreamsMetricsImpl - but again, it seems to me that
there are enough code smells to indicate that something may need to be
re-evaluated a bit further.
With those points I believe that this ticket needs to be evaluated a bit more
deeply - since it is likely connected to some considerable tech-debt (e.g.
final methods mocking, inconsistencies on how the mocking of the metrics is
performed, high coupling with implementation instead of interface, etc).
Unfortunately I won’t be able to provide the amount fo rework/redesign that
this ticket requires right now, maybe a core committer could eventually look
into it if important?
was (Author: marcolotz):
[~mjsax] [~cadonna] I have spent a reasonable amount of time analysing this
ticket. It seems to me that this is tightly coupled in many points (which also
explains the fair amount of unmerged PRs related to it). Based on the previous
comments, I found out the following in the code:
* As [~chia7712] mentioned, if we keep the MockStreamsMetrics class and make
it implement the interface, we would need to extend the interface (thus a KIP).
Specially because the MockStreamsMetrics is used interchangeably with
StreamsMetricsImpl class. The implementation has tons of extra public methods.
Including static ones. There is a huge amount of test classes (20+) counting on
StreamsMetricsImpl behaviours provided by MockStreamsMetrics.
* If we go for removing MockStreamsMetrics and trying to mock
StreamsMetricsImpl whenever it’s not truly needed:
* One will find out that many methods in the class are final (which is not a
limitation since PowerMock is available in Kafka project). But is a sign that
it may require further design attention. Among them is
{code:java}
public final Sensor taskLevelSensor(...){code}
* One will uncover that many static final methods are also called for
StreamsMetricsImpl (that can also be solved by PowerMock). Among them is:
{code:java}
public static void StreamsMetricsImpl.addValueMetricToSensor(...){code}
* One will find out that many test classes (e.g. StreamTaskTest or
RecordCollectorTest) would require a considerable amount of behaviour mocking.
Seems easily 20+ test classes would need considerable amount of mock
configuration in this scenario. A possible solution is indeed making a mocking
utils to init mocks of StreamsMetricsImpl - but again, it seems to me that
there are enough code smells to indicate that something may need to be
re-evaluated a bit further.
With those points I believe that this ticket needs to be evaluated a bit more
deeply - since it is likely connected to some considerable tech-debt (e.g.
final methods mocking, inconsistencies on how the mocking of the metrics is
performed, high coupling with implementation instead of interface, etc).
Unfortunately I won’t be able to provide the amount fo rework/redesign that
this ticket requires right now, maybe a core committer could eventually look
into it if important?
> MockStreamsMetrics should be in o.a.k.test
> ------------------------------------------
>
> Key: KAFKA-5676
> URL: https://issues.apache.org/jira/browse/KAFKA-5676
> Project: Kafka
> Issue Type: Bug
> Components: streams
> Reporter: Guozhang Wang
> Priority: Major
> Labels: newbie
> Time Spent: 96h
> Remaining Estimate: 0h
>
> {{MockStreamsMetrics}}'s package should be `o.a.k.test` not
> `o.a.k.streams.processor.internals`.
> In addition, it should not require a {{Metrics}} parameter in its constructor
> as it is only needed for its extended base class; the right way of mocking
> should be implementing {{StreamsMetrics}} with mock behavior than extended a
> real implementaion of {{StreamsMetricsImpl}}.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
