Include empty servers in BrokerServerView.#18200
Conversation
Fixes apache#18199, because this makes empty Historicals visible through getDruidServerMetadatas. It also makes them visible through getDruidServers, causes them to show up in the sys.servers table.
| private void runServerCallbacks(final Function<ServerCallback, CallbackAction> fn) | ||
| { | ||
| for (final Map.Entry<ServerRemovedCallback, Executor> entry : serverRemovedCallbacks.entrySet()) { | ||
| for (final Map.Entry<ServerCallback, Executor> entry : serverCallbacks.entrySet()) { |
There was a problem hiding this comment.
Nit: Probably cleaner to use Map.forEach.
There was a problem hiding this comment.
Personally, I prefer the regular for in situations where the forEach can't be a one-liner (& even sometimes then) so I kept it as-is.
| @Override | ||
| public CallbackAction serverAdded(DruidServer server) | ||
| { | ||
| if (!server.getType().equals(ServerType.BROKER)) { |
There was a problem hiding this comment.
Please add a 1-liner comment like "Do not keep inventory of other Brokers" or something.
There was a problem hiding this comment.
Added a comment We don't track brokers in this view.
| if (!server.getType().equals(ServerType.BROKER)) { | ||
| addServer(server); | ||
| } | ||
| return CallbackAction.CONTINUE; |
There was a problem hiding this comment.
Do we ever use the other callback action UNREGISTER?
I think it is not used anymore and can probably be removed.
There was a problem hiding this comment.
It's not used, & seems like it hasn't been used since #6742. I suppose we could remove it in this PR or in a new one.
| public interface ServerView | ||
| { | ||
| void registerServerRemovedCallback(Executor exec, ServerRemovedCallback callback); | ||
| void registerServerCallback(Executor exec, ServerCallback callback); |
There was a problem hiding this comment.
Super nit:
To allow for more concise syntax (using lambdas) in the calling code, I wonder if we should have an overloaded method with the signature:
void registerServerCallback(Executor exec, Consumer<DruidServer> onAdded, Consumer<DruidServer> onRemoved);assuming we get rid of the CallbackAction altogether.
ServerCallback can then be a concrete class which takes two Consumer<DruidServer> in its constructor, default values for which would be no-op.
It might be useful even when we add new methods to ServerCallback as callers would need to override only the required methods.
There was a problem hiding this comment.
A builder might be nice too. It would allow keeping together all the ServerCallback stuff in the interface. Something like:
ServerCallback.builder()
.onServerAdded(server -> ...)
.onServerRemoved(server -> ...)
.build()or for just doing one:
ServerCallback.onServerRemoved(server -> ...)etc.
Would like to have this be a different PR, so that PR can focus on the thinking about the best way to specify callbacks that only want to listen to one thing.
2 participants
Fixes #18199, because this makes empty Historicals visible through getDruidServerMetadatas. It also makes them visible through getDruidServers, causes them to show up in the sys.servers table.