[jira] [Comment Edited] (KAFKA-5676) MockStreamsMetrics should be in o.a.k.test

["Marco Lotz (Jira)" <ji...@apache.org>]

    [ 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:04 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}


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. 

> 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)