separate ingestion and query thread pool

[pjain1]

When the ingestion and query rate for a realtime index task using the event receiver firehose is quite high, then having different thread pools for handling ingestion and querying will be quite useful. The separation will be useful in case ingestion and querying takes a while to respond for example deserializing sketch objects during ingestion or responding to a slow query. Thus, the ingestion cannot not block queries and vice-versa as they can be tuned independently.

[drcrallen]

That's awesome, the power of annotations

[pjain1]

👍 thanks to @cheddar for showing how to do this

[pjain1]

@xvrl @drcrallen took care of comments. Using consecutive ports for jetty servers.

[drcrallen]

Where is this consumed?

[pjain1]

These three properties are used to create DruidNode object annotated with RemoteChatHandler at ServiceAnnouncingChatHandlerProvider and getServer method of ChatHandlerServerModule. However, while announcement of the event receiver firehose the service name provided during registration is used and the 'serviceName' property which is specified here is ignored. Thus, it is not consumed anywhere, it is just there so that object creation does not fail.

[drcrallen]

If that's so can you make it something that won't register as a valid ZK path? also please add a comment as to why it is set, and where the property actually gets taken from.

I'd hate for some change to happen later to the chat handler and have this passively create new ZK nodes without intending to, all because we forgot it was here.

[pjain1]

sure...makes sense

[pjain1]

actually I don't think I can change it somehow so that it does not register as valid ZKPath as if the peon were to somehow use this serviceName, it will just take the String as it is and try to announce it. What can be done here as you suggested to change the name to something like placeholderServiceName and add a comment form where the actual serviceName will be taken ?

[pjain1]

Nice, thanks. Should have thought about it, anyways done

[pjain1]

wait...it may not help as serviceName is not a path because all the "/" are replaced to ":" and the resultant String is used to announce the service..let me double check

[pjain1]

I checked putting invalid path as serviceName will not help, makeCanonicalServiceName method in CuratorServiceUtils class replaces all "" with ":"
However I have put another character \u0001 (invalid for zk) in the String and it will fail if this name is used. Also I have added some UTs for ServiceAnnouncer to test the behavior. Please review.

[drcrallen]

awesome! thanks

[pjain1]

I ran integration tests with this change and actually even the deserialization of DruidNode failed when the peon process starts with exception - com.fasterxml.jackson.core.JsonParseException: Illegal unquoted character ((CTRL-CHAR, code 1)): has to be escaped using backslash to be included in string value
So I think it is just better to have only comment there explaining this name would be ignored and not do anything smart.

[drcrallen]

Cool, 👍 after travis

[pjain1]

Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 7.062 sec <<< FAILURE! - in io.druid.curator.announcement.AnnouncerTest
testSessionKilled(io.druid.curator.announcement.AnnouncerTest)  Time elapsed: 2.793 sec  <<< FAILURE!
java.lang.AssertionError: expected null, but was:<10,10,1447436227166,1447436227166,0,0,0,94859180405555201,25,0,10>
    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failNotNull(Assert.java:664)
    at org.junit.Assert.assertNull(Assert.java:646)
    at org.junit.Assert.assertNull(Assert.java:656)
    at io.druid.curator.announcement.AnnouncerTest.testSessionKilled(AnnouncerTest.java:173)

closing/reopening again for travis

[pjain1]

@drcrallen travis is happy

[pjain1]

@xvrl any more comments ?

[xvrl]

endpoint is one word

[pjain1]

ok

[pjain1]

Transient failure -

io.druid.indexing.overlord.TaskLifecycleTest
testRealtimeIndexTask[taskStorageType=HeapMemoryTaskStorage](io.druid.indexing.overlord.TaskLifecycleTest)  Time elapsed: 2.379 sec  <<< FAILURE!
java.lang.AssertionError: null
    at org.junit.Assert.fail(Assert.java:86)
    at org.junit.Assert.assertTrue(Assert.java:41)
    at org.junit.Assert.assertTrue(Assert.java:52)
    at io.druid.indexing.overlord.TaskLifecycleTest.testRealtimeIndexTask(TaskLifecycleTest.java:833)

I have refactored this test in #1679 and I think it should solve this transient error

[himanshug]

@NotNull is probably not helping as boolean can never be null anyway

[pjain1]

ok

[pjain1]

done

[himanshug]

👍