[
https://issues.apache.org/jira/browse/KAFKA-5676?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17349475#comment-17349475
]
Marco Lotz edited comment on KAFKA-5676 at 5/21/21, 9:39 PM:
-------------------------------------------------------------
[~mjsax] [~guozhang] I can take over this ticket if nobody minds. I had a look
at the code and indeed there's quite a problem of dependency inversion here.
Tons of classes using behaviours only available in StreamsMetricsImpl instead
of StreamsMetrics. Among the public behaviours not declared in the interface
are:
{code:java}
Version version();
addClientLevelMutableMetric();
and many more{code}
An example of calls that directly use the implementation is in
ProcessorNodeMetrics#addStateMetric
{code:java}
public static void addStateMetric(final StreamsMetricsImpl streamsMetrics,
final Gauge<State> stateProvider) {
streamsMetrics.addClientLevelMutableMetric( <--- This one is only available
in the implementation
STATE,
STATE_DESCRIPTION,
RecordingLevel.INFO,
stateProvider
);
}
{code}
It seems to me that the interface StreamMetrics is missing many behaviours. I
can work on a PR to move those behaviours up to the interface and then we work
together from there.
Edit: Extra example, even interfaces are returning the implementation:
{code:java}
public interface InternalProcessorContext {
...
@Override
StreamsMetricsImpl metrics();
}{code}
Edit 2: It seems to be many inconsistencies around, there are far more calls to
{code:java}
createMock(StreamsMetricsImpl.class);{code}
and similar calls than uses of MockStreamsMetrics class. This will be likely a
huge PR if this is really the goal. But then, seeing that there's only one
implementation of the interface and the implementation has so many "extra"
behaviours, the following question comes: is having interface segregation here
the way to go? If it is, then we need to revisit the interface behaviours. If
it's not, then maybe the interface should be dropped in favour to the single
implementation.
was (Author: marcolotz):
[~mjsax] [~guozhang] I can take over this ticket if nobody minds. I had a look
at the code and indeed there's quite a problem of dependency inversion here.
Tons of classes using behaviours only available in StreamsMetricsImpl instead
of StreamsMetrics. Among the public behaviours not declared in the interface
are:
{code:java}
Version version();
addClientLevelMutableMetric();
and many more{code}
An example of calls that directly use the implementation is in
ProcessorNodeMetrics#addStateMetric
{code:java}
public static void addStateMetric(final StreamsMetricsImpl streamsMetrics,
final Gauge<State> stateProvider) {
streamsMetrics.addClientLevelMutableMetric( <--- This one is only available
in the implementation
STATE,
STATE_DESCRIPTION,
RecordingLevel.INFO,
stateProvider
);
}
{code}
It seems to me that the interface StreamMetrics is missing many behaviours. I
can work on a PR to move those behaviours up to the interface and then we work
together from there.
Edit: Extra example, even interfaces are returning the implementation:
{code:java}
public interface InternalProcessorContext {
...
@Override
StreamsMetricsImpl metrics();
}{code}
> 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
> Assignee: Marco Lotz
> 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)
