kfaraz commented on code in PR #18200:
URL: https://github.com/apache/druid/pull/18200#discussion_r2184163060
##########
server/src/main/java/org/apache/druid/client/BatchServerInventoryView.java:
##########
@@ -243,13 +244,13 @@ protected void runSegmentCallbacks(
}
}
- private void runServerRemovedCallbacks(final DruidServer server)
+ 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()) {
Review Comment:
Nit: Probably cleaner to use `Map.forEach`.
##########
server/src/main/java/org/apache/druid/client/BrokerServerView.java:
##########
@@ -157,11 +157,24 @@ public CallbackAction
segmentSchemasAnnounced(SegmentSchemas segmentSchemas)
segmentFilter
);
- baseView.registerServerRemovedCallback(
+ baseView.registerServerCallback(
exec,
- server -> {
- removeServer(server);
- return CallbackAction.CONTINUE;
+ new ServerCallback() {
+ @Override
+ public CallbackAction serverAdded(DruidServer server)
+ {
+ if (!server.getType().equals(ServerType.BROKER)) {
Review Comment:
Please add a 1-liner comment like "Do not keep inventory of other Brokers"
or something.
##########
server/src/main/java/org/apache/druid/client/ServerView.java:
##########
@@ -29,7 +29,7 @@
*/
public interface ServerView
{
- void registerServerRemovedCallback(Executor exec, ServerRemovedCallback
callback);
+ void registerServerCallback(Executor exec, ServerCallback callback);
Review 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:
```java
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.
##########
server/src/main/java/org/apache/druid/client/BrokerServerView.java:
##########
@@ -157,11 +157,24 @@ public CallbackAction
segmentSchemasAnnounced(SegmentSchemas segmentSchemas)
segmentFilter
);
- baseView.registerServerRemovedCallback(
+ baseView.registerServerCallback(
exec,
- server -> {
- removeServer(server);
- return CallbackAction.CONTINUE;
+ new ServerCallback() {
+ @Override
+ public CallbackAction serverAdded(DruidServer server)
+ {
+ if (!server.getType().equals(ServerType.BROKER)) {
+ addServer(server);
+ }
+ return CallbackAction.CONTINUE;
Review Comment:
Do we ever use the other callback action `UNREGISTER`?
I think it is not used anymore and can probably be removed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
