Kafka-5676: MockStreamsMetrics should be in o.a.k.test #3657
Conversation
DevelopersWithPassion
commented
Aug 11, 2017
DevelopersWithPassion
commented
- Change package for MockStreamsMetrics class
- Removed metric object from MockStreamsMetrics constcruor
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
DevelopersWithPassion
changed the title
|
Thanks for the PR @DevelopersWithPassion Could you rebase with the latest trunk? |
| @@ -25,7 +24,7 @@ | |||
| import org.apache.kafka.streams.kstream.SessionWindows; | |||
| import org.apache.kafka.streams.kstream.Windowed; | |||
| import org.apache.kafka.streams.processor.Processor; | |||
| import org.apache.kafka.streams.processor.internals.MockStreamsMetrics; | |||
| import org.apache.kafka.test.MockStreamsMetrics; | |||
There was a problem hiding this comment.
nit: reorder the imports alphabetically
There was a problem hiding this comment.
sure will do that.
| import org.apache.kafka.test.MockStateStoreSupplier; | ||
| import org.apache.kafka.test.MockTimestampExtractor; | ||
| import org.apache.kafka.test.TestUtils; | ||
| import org.apache.kafka.test.*; |
There was a problem hiding this comment.
We do not recommend using wildcards in imports.
There was a problem hiding this comment.
ok .I will change it
| public MockStreamsMetrics(Metrics metrics) { | ||
| super(metrics, "mock-stream-metrics", | ||
| public MockStreamsMetrics() { | ||
| super(new Metrics(), "mock-stream-metrics", |
There was a problem hiding this comment.
So far this mock object is still actually implementing the full functionalities of a StreamsMetricsImpl since the mock actually extends it; the "mocking" is that it is passing in a default group name and an empty tag map.
A real "mock" class, would be not implementing any real functionalities (so not needing an underlying Metrics object) but just implementing the StreamsMetrics interface as no-ops, or at best recording the times that some APIs has been called etc. For cases that really needs a functional StreamsMetricsImpl object, we should just pass it instead of the mocking object. Please see my comments in the related class.
There was a problem hiding this comment.
Ok let me try other way . I have to change lot of test cases . Thanks for the information :-)
| @@ -115,7 +109,8 @@ | |||
| private final byte[] recordKey = intSerializer.serialize(null, 1); | |||
| private final String applicationId = "applicationId"; | |||
| private final Metrics metrics = new Metrics(); | |||
| private final StreamsMetrics streamsMetrics = new MockStreamsMetrics(metrics); | |||
| private final StreamsMetrics streamsMetrics = new StreamsMetricsImpl(metrics, "mock-stream-metrics", | |||
There was a problem hiding this comment.
As we mentioned in the ticket, the only test case that requires the underlying metrics object is testMetrics. For this purpose we should just rewrite this test function, to not reuse the class field task which is relying on the MockStreamsMetrics, but create another task that does pass in a real StreamsMetricsImpl.
| @@ -45,7 +44,7 @@ | |||
|
|
|||
| @Before | |||
| public void setUp() throws Exception { | |||
| streamMetrics = new MockStreamsMetrics(new Metrics()); | |||
| streamMetrics = new MockStreamsMetrics(); | |||
There was a problem hiding this comment.
Similarly as in StreamTaskTest, we should just use a different cache object in the testMetrics function than the class field cache which gets a real StreamsMetricsImpl directly.
There was a problem hiding this comment.
Similarly for ProcessorNodeTest: the only difference is that for that test case we can introduce another overloaded constructor for MockProcessorContext which takes a StreamsMetrics object, and then pass a real StreamsMetricsImpl only for that testMetrics class while for all others just use the other constructor which always use a MockStreamsMetrics object.
|
Also you can rename your PR title as |
DevelopersWithPassion
changed the title
|
@DevelopersWithPassion are you still working on this PR? |
|
@guozhangwang no i am not working on it . will continue after some time . please assign it to somebody if priority has changed |
|
Just checking, no priority changed for now :) |
|
Closing this old PR due to inactivity. PR would need to be rebased anyway. |
