Add a ServerType for peons

[jon-wei]

I encountered a failure in a kafka indexing task after #4148 introduced ServerType:

Caused by: java.lang.IllegalArgumentException: No enum constant io.druid.server.coordination.ServerType.INDEXER-EXECUTOR
	at java.lang.Enum.valueOf(Enum.java:238)
	at io.druid.server.coordination.ServerType.valueOf(ServerType.java:22)
	at io.druid.server.coordination.ServerType.fromString(ServerType.java:58)
	at io.druid.server.coordination.DruidServerMetadata.<init>(DruidServerMetadata.java:50)
	at io.druid.guice.StorageNodeModule.getMetadata(StorageNodeModule.java:71)
	at io.druid.guice.StorageNodeModule$$FastClassByGuice$$a8289f44.invoke(<generated>)
...
	at io.druid.guice.LifecycleModule$2.start(LifecycleModule.java:154)
	at io.druid.cli.GuiceRunnable.initLifecycle(GuiceRunnable.java:103)
	at io.druid.cli.CliPeon.run(CliPeon.java:279)
	at io.druid.cli.Main.main(Main.java:109)

This PR adds a ServerType for peons which have a nodeType of "indexer-executor".

[leventov]

_/- conversion is a thin ice. There is an number of places where enum name in converted back to string: BatchDataServerAnnouncer, ServersResource. Could it be avoided altogether, by giving INDEXER_/-EXECUTOR a single-word name?

Also there is a switch in DruidCluster which checks ServerType, please confirm it shouldn't be changed.

[jihoonson]

@jon-wei thanks. I missed this server type. The code looks good overall. Would you address @leventov's comment?

[jon-wei]

Thanks for the review!

@leventov
Re: the enum conversion, good catch on the back conversions, though I'm not sure what the best approach is here, I could override toString() for the peon type such that it returns "indexer-executor" or do the single-word change.

I initially considered renaming the nodeType to one word or switching the hyphen to an underscore, but I'm not familiar with the Druid-Zookeeper interactions, and wasn't clear on whether the rename would break something related to node/path handling there.

I'm also wary of changing it for external compatibility reasons, I see at least one external piece of code that appears to assume that nodeType name, not sure if there are users out there that rely on the "indexer-executor" string:
https://github.com/druid-io/druid-console/blob/master/src/client/factories/hUtils.coffee#L25

---

If someone has a clear view on the impact of changing the nodeType string, feedback would be much appreciated.

---

Re: the switch in DruidCluster, throwing an error in the switch for the peon type is fine since the peons are not intended to be segment replication targets.

[jihoonson]

Re: the switch in DruidCluster, throwing an error in the switch for the peon type is fine since the peons are not intended to be segment replication targets.

Throwing an exception looks fine, but it would be better to improve the error message. Currently it simply complains about unknown server type, but I think it should be unsupported server type.

[jon-wei]

@leventov @jihoonson

Updated the patch:

• Added a case to the switch statement for INDEXER_EXECUTOR that throws an exception with "Unsupported server type"

• Overrode toString() method for ServerType to lowercase and convert underscores to hyphens, ended up deciding to keep the old format for backwards compatibility

• Adds a fix to ServersResource so that ServerType.toString() is used for the "type" field in server info output instead of the enum object itself, this previously broke the coordinator console's server view

[jihoonson]

@jon-wei thanks! It looks good. I left a simple comment.

[jihoonson]

This might be not valid because INDEXER_EXECUTOR cannot hold segments?

[jon-wei]

The peons will hold segments for some time as they're indexing, before they're handed off

[jihoonson]

Ah, it makes sense.

[leventov]

Shouldn't you annotate fromString() as @JsonCreator? Or it works already?

[jon-wei]

I don't think there are instances where a ServerType is being deserialized directly, DruidServer and DruidServerMetadata would be such locations but they currently take a String "type" and DruidServerMetadata converts that String to a ServerType with fromString().

I'll look into adding @ JsonCreator here and changing DruidServer/DruidServerMetadata to accept a ServerType parameter.

[pjain1]

This looks good to me. @jon-wei are you planning to add anything more in this for @leventov's comment ?

[jon-wei]

@pjain1 Thanks, I'm going to push another update that fixes some unit test failures and the String->ServerType changes I mentioned

[pjain1]

@jon-wei thanks looking forward to it, lets get this fixed soon.

[jon-wei]

Added @JsonCreator and @JsonValue to fromString()/toString() in ServerType, changed DruidServer and DruidServerMetadata to accept a ServerType parameter instead of a type string

[pjain1]

👍