You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2023/11/24 07:53:11 UTC
[PR] Use StubServiceEmitter in tests (druid)
kfaraz opened a new pull request, #15426:
URL: https://github.com/apache/druid/pull/15426
### Changes
- Replace other adhoc test implementations of `ServiceEmitter` with existing `StubServiceEmitter`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use StubServiceEmitter in tests (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #15426:
URL: https://github.com/apache/druid/pull/15426#discussion_r1407055481
##########
server/src/test/java/org/apache/druid/curator/DruidConnectionStateListenerTest.java:
##########
@@ -120,31 +104,8 @@ public void test_reconnectedMetric()
listener.stateChanged(null, ConnectionState.RECONNECTED);
Assert.assertEquals(2, emitter.getEvents().size()); // the second stateChanged emits a metric
- final Map<String, Object> eventMap = emitter.getEvents().get(1).toMap();
- Assert.assertEquals("metrics", eventMap.get("feed"));
- Assert.assertEquals("zk/reconnect/time", eventMap.get("metric"));
- MatcherAssert.assertThat(eventMap.get("value"), CoreMatchers.instanceOf(Long.class));
- MatcherAssert.assertThat(((Number) eventMap.get("value")).longValue(), Matchers.greaterThanOrEqualTo(0L));
+ long observedReconnectTime = emitter.getValue("zk/reconnect/time", null).longValue();
+ Assert.assertTrue(observedReconnectTime >= 0);
}
- private static class TestEmitter extends NoopServiceEmitter
- {
- @GuardedBy("events")
- private final List<Event> events = new ArrayList<>();
-
- @Override
- public void emit(Event event)
- {
- synchronized (events) {
Review Comment:
Yeah, maybe it was just copied over from someplace where it was actually needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use StubServiceEmitter in tests (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #15426:
URL: https://github.com/apache/druid/pull/15426#discussion_r1405861536
##########
server/src/test/java/org/apache/druid/curator/DruidConnectionStateListenerTest.java:
##########
@@ -120,31 +104,8 @@ public void test_reconnectedMetric()
listener.stateChanged(null, ConnectionState.RECONNECTED);
Assert.assertEquals(2, emitter.getEvents().size()); // the second stateChanged emits a metric
- final Map<String, Object> eventMap = emitter.getEvents().get(1).toMap();
- Assert.assertEquals("metrics", eventMap.get("feed"));
- Assert.assertEquals("zk/reconnect/time", eventMap.get("metric"));
- MatcherAssert.assertThat(eventMap.get("value"), CoreMatchers.instanceOf(Long.class));
- MatcherAssert.assertThat(((Number) eventMap.get("value")).longValue(), Matchers.greaterThanOrEqualTo(0L));
+ long observedReconnectTime = emitter.getValue("zk/reconnect/time", null).longValue();
+ Assert.assertTrue(observedReconnectTime >= 0);
}
- private static class TestEmitter extends NoopServiceEmitter
- {
- @GuardedBy("events")
- private final List<Event> events = new ArrayList<>();
-
- @Override
- public void emit(Event event)
- {
- synchronized (events) {
Review Comment:
note: I wonder why these `syncronized` blocks were added (history doesn't explain) ...they don't seem necessary as `doMonitor` is explicitly called in this test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use StubServiceEmitter in tests (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz merged PR #15426:
URL: https://github.com/apache/druid/pull/15426
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org