internal-discovery: interfaces for announcement/discovery, curator based impls

[himanshug]

This PR introduces formal core "internal-discovery" in Druid. It should be used by any Druid node to discover any other node inside druid cluster. a curator/zookeeper based default implementation is included in druid core but it is extensible to provide extension that might not use zookeeper.

• For all core classes/interfaces , see server/src/main/java/io/druid/discovery/*.java

• For curator based impls, see server/src/main/java/io/druid/curator/discovery/CuratorDruidNode*.java

• All CliXXX.java classes are updated to announce themselves in "internal-discovery".

• RealtimeIndexTask and KafkaIndexTask are updated to announce themselves in "internal-discovery".

• HttpServerInventoryView is updated to discover data nodes using "internal-discovery"

• /druid/coordinator/v1/cluster and /druid/coordinator/v1/cluster/<nodeType> endpoints are introduced at coordinator to get a dump of nodes in "internal-discovery", this is intended mostly to be used for manual testing/debugging for now.

TODO:

• add/update unit tests
• fix style checks if needed

Follow-up PRs:

• Update coordinator to discover lookup nodes using "Internal-discovery" ( update internal-discovery Listener for node list and use same at router and coordinator #4697 )
• Update router to discover broker nodes using "internal-discovery" ( update internal-discovery Listener for node list and use same at router and coordinator #4697 )
• Add interfaces and curator impls for leader elections and use same in DruidCoordinator/TaskMaster. ( DruidLeaderSelector interface for leader election and Curator based impl. #4699 )
• In a later release, use "internal-discovery" to discover coordinator/overlord leader at clients in peon etc.
• In a later release, remove the dead code used for existing internal announcements and discovery.

[cheddar]

No docs?

[Himanshu] No new configs are introduced in this PR.

[cheddar]

I think it would be nicer to take the informatino for this in the task spec instead of runtime config. That would give us the ability to set it on a per-task basis.

[himanshug]

lookupTier is something that user might want to override so made it overridable via task context key lookupTier in Realtime and Kafka indexing tasks.

[cheddar]

typo!

[himanshug]

fixed

[cheddar]

Nit: I read "/" as "OR" which seems weird where we are both creating and starting. I think the message would be better as just "Creating NodeTypeWatcher" or "Creating and starting NodeTypeWatcher"

[himanshug]

changed

[cheddar]

I don't think you need to do the get here, just return nodeTypeWatcher

[himanshug]

changed

[cheddar]

Nit: I hate synchronized methods. It's very easy to overlook and can also blur the boundary of what is being synchronized (if someone external from this class uses this class for synchronization, all of a sudden they will be fighting with the internals of this for mutual exclusion).

I think it would be better to just create a private "lock" Object and synchronize on that instead. I care about it enough to comment, but not enough to actually enforce so do as you will.

[himanshug]

removed synchronized from method

[cheddar]

Collections.unmodifiableSet()

[cheddar]

It might be nice to have endpoints that just return one type of service and stuff do. While maybe not necessary, it would be a relatively simple addition that benefits the operator (I think). Or, take a parameter to filter which types you care about, or something similar.

[himanshug]

yeah, i added this one quickly just to be able to degugging and manual verification... but more features can be added here.
that said, added another endpoint /druid/coordinator/v1/<nodeType> to return all the nodes for given node type.

[cheddar]

Not asEagerSingleton, asEagerSingleton causes weird oddities in Guice land. Use Lifecycle.register(ForSideEffectsOnlyProvider.Child.class) instead.

[himanshug]

refactored it , asEagerSingleton() is not used anymore.

[cheddar]

Do this in the get() call instead of here. Setting this stuff up in the constructor (especially when marked as an eagerSingleton) can lead to weird and interesting interleaving of injections.

[himanshug]

refactored it , asEagerSingleton() is not used anymore.

[cheddar]

It's kinda annoying that you have a provider in each of these CLIs. The only deviation between these is the node type and the map. You should be able to create a single provider that takes on the constructor a String nodeType, List<Key<DruidService>> services. Move the injections to an

@Inject
public void init(Injector injector, Lifecycle lifecycle, ...)

Then in the get() you would walk the list of Keys, asking the injector to inject them, build the map out of those and then use that to build the discoveryDruidNode.

Once that is setup, your module code would look like

binder.bind(ForSideEffectsOnlyProvider.Child.class).toProvider(new ForSideEffectsOnlyProvider("historical", Arrays.asList(Key.get(DataNodeService.class, LookupNodeService.class)));
LifecycleModule.register(ForSideEffectsOnlyProvider.Child.class);

[himanshug]

refactored it , asEagerSingleton() is not used anymore. Individual CliXXX classes don't have much now.