You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/02/22 19:37:36 UTC
[GitHub] [druid] gianm commented on a change in pull request #12222: Allow extension services to be discovered
gianm commented on a change in pull request #12222:
URL: https://github.com/apache/druid/pull/12222#discussion_r812200296
##########
File path: docs/operations/api-reference.md
##########
@@ -935,6 +935,12 @@ Returns segment information lists including server locations for the given query
#### GET
+* `/druid/v2/router/cluster`
Review comment:
Traditionally `/druid/v2/` is a root for Broker APIs. I think the most-proper home for this API would be under `/druid/router/v1/cluster`.
##########
File path: services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java
##########
@@ -79,6 +79,7 @@
/**
* This class does async query processing and should be merged with QueryResource at some point
*/
+@SuppressWarnings("serial")
Review comment:
Shouldn't be needed, since this class isn't Serializable, and anyway we don't use or care about Java serialization here (or, hopefully, anywhere).
##########
File path: services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java
##########
@@ -648,6 +650,7 @@ static String getAvaticaProtobufConnectionId(Service.Request request)
private class MetricsEmittingProxyResponseListener<T> extends ProxyResponseListener
{
private final HttpServletRequest req;
+ @SuppressWarnings("unused")
Review comment:
If it's not used, better to remove it, I think.
##########
File path: server/src/main/java/org/apache/druid/discovery/NodeRoles.java
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.discovery;
+
+import com.google.common.collect.Collections2;
+import com.google.inject.Binder;
+import com.google.inject.multibindings.Multibinder;
+import org.apache.druid.guice.annotations.Global;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.Node;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Set;
+
+public class NodeRoles
+{
+ private static final Logger LOG = new Logger(NodeRoles.class);
+
+ /**
+ * Simulate the Guice binding of all node roles, but using just
+ * the known roles. Primarily for testing.
+ */
+ public static Set<NodeRole> knownRoles()
+ {
+ return new HashSet<>(Arrays.asList(NodeRole.values()));
+ }
+
+ public static void addKnownRoles(Binder binder)
+ {
+ Multibinder<NodeRole> roleBinder = binder(binder);
+ for (NodeRole role : NodeRole.values()) {
+ roleBinder.addBinding().toInstance(role);
+ }
+ }
+
+ /**
+ * Add a node role for an extension service.
+ */
+ public static void addRole(Binder binder, NodeRole role)
+ {
+ LOG.debug("Adding node role: " + role.getJsonName());
Review comment:
Minor: it's a bit more conventional, Druid-wise, to do `log.debug("Adding node role: %s", role.getJsonName())`.
##########
File path: server/src/main/java/org/apache/druid/discovery/LocalDruidNodeDiscoveryProvider.java
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.discovery;
+
+import org.apache.druid.server.DruidNode;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.BooleanSupplier;
+
+/**
+ * Local node discovery. In this mode, Druid may run a number of
+ * logical servers within a single process, so that node discovery
+ * is all within the same process, but matches a true distributed
+ * node discovery.
+ * <p>
+ * Concurrency is at the level of the entire provider, which is
+ * necessary to coordinate the active state with ongoing node
+ * discovery and listener registrations. Such a "global" lock
+ * is fine because the actions don't occur frequently, nor do the
+ * actions take much time.
+ */
+public class LocalDruidNodeDiscoveryProvider extends DruidNodeDiscoveryProvider implements DruidNodeAnnouncer
+{
+ private class RoleEntry implements DruidNodeDiscovery
+ {
+ @SuppressWarnings("unused")
+ private final NodeRole role;
Review comment:
Why have this private field if it's unused?
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java
##########
@@ -674,9 +684,9 @@ private static DruidServer toDruidServer(DiscoveryDruidNode discoveryDruidNode)
}
}
- private static Iterator<DiscoveryDruidNode> getDruidServers(DruidNodeDiscoveryProvider druidNodeDiscoveryProvider)
+ private Iterator<DiscoveryDruidNode> getDruidServers(DruidNodeDiscoveryProvider druidNodeDiscoveryProvider)
{
- return Arrays.stream(NodeRole.values())
+ return allNodeRoles.stream()
Review comment:
Hmm, I suppose this means that if an extension adds a node role that the Broker doesn't (yet?) know about, then it won't show up in system tables. There'll potentially be a window of "invisibility" before the Broker is updated to include the extension. And in particular, all extensions that add node roles need to be loaded on the Broker sooner or later.
Personally, I think this is OK, but it deserves a mention on the javadocs for NodeRole.
##########
File path: server/src/main/java/org/apache/druid/discovery/LocalDruidNodeDiscoveryProvider.java
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.discovery;
+
+import org.apache.druid.server.DruidNode;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.BooleanSupplier;
+
+/**
+ * Local node discovery. In this mode, Druid may run a number of
+ * logical servers within a single process, so that node discovery
+ * is all within the same process, but matches a true distributed
+ * node discovery.
+ * <p>
+ * Concurrency is at the level of the entire provider, which is
+ * necessary to coordinate the active state with ongoing node
+ * discovery and listener registrations. Such a "global" lock
+ * is fine because the actions don't occur frequently, nor do the
+ * actions take much time.
+ */
+public class LocalDruidNodeDiscoveryProvider extends DruidNodeDiscoveryProvider implements DruidNodeAnnouncer
Review comment:
I don't see a way to activate this class outside of unit tests. Is it just meant for unit testing? If so, please call that out in the javadocs, and consider moving it to test scope. Or, if it's meant to be something we'll one day use outside of unit tests, please call that out in the javadocs, along with a reason as to why it isn't exposed yet.
##########
File path: server/src/main/java/org/apache/druid/initialization/Initialization.java
##########
@@ -517,7 +519,13 @@ private boolean shouldLoadOnCurrentNodeType(Object object)
Set<NodeRole> rolesPredicate = Arrays.stream(loadScope.roles())
.map(NodeRole::fromJsonName)
.collect(Collectors.toSet());
- return rolesPredicate.stream().anyMatch(nodeRoles::contains);
+ boolean shouldLoad = rolesPredicate.stream().anyMatch(nodeRoles::contains);
+ if (!shouldLoad) {
+ log.info(
Review comment:
Better to have this at `debug` level. It'll pollute log messages otherwise.
##########
File path: docs/operations/api-reference.md
##########
@@ -935,6 +935,12 @@ Returns segment information lists including server locations for the given query
#### GET
+* `/druid/v2/router/cluster`
+
+Returns a list of the servers registered within the cluster. Similar to
Review comment:
Would the response format of this API be similar to, or exactly the same as, `/druid/coordinator/v1/cluster`? Ideally, if it's exactly the same as, we should say that; if it's merely similar we should outline the differences.
##########
File path: server/src/main/java/org/apache/druid/initialization/Initialization.java
##########
@@ -198,6 +199,7 @@ private void addAllFromFileSystem()
ServiceLoader.load(serviceClass, loader).forEach(impl -> tryAdd(impl, "local file system"));
}
catch (Exception e) {
+ log.error(e, "Failed to load extension [%s]", serviceClass.getName());
Review comment:
Wondering why this is necessary. Will the RuntimeException below get chomped by something? Usually if there's a fatal exception on startup we get a stack trace for it.
##########
File path: server/src/main/java/org/apache/druid/discovery/LocalDruidNodeDiscoveryProvider.java
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.discovery;
+
+import org.apache.druid.server.DruidNode;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.BooleanSupplier;
+
+/**
+ * Local node discovery. In this mode, Druid may run a number of
+ * logical servers within a single process, so that node discovery
+ * is all within the same process, but matches a true distributed
+ * node discovery.
+ * <p>
+ * Concurrency is at the level of the entire provider, which is
+ * necessary to coordinate the active state with ongoing node
+ * discovery and listener registrations. Such a "global" lock
+ * is fine because the actions don't occur frequently, nor do the
+ * actions take much time.
+ */
+public class LocalDruidNodeDiscoveryProvider extends DruidNodeDiscoveryProvider implements DruidNodeAnnouncer
+{
+ private class RoleEntry implements DruidNodeDiscovery
+ {
+ @SuppressWarnings("unused")
+ private final NodeRole role;
+ private final Map<DruidNode, DiscoveryDruidNode> nodes = new HashMap<>();
+ private final List<Listener> listeners = new ArrayList<>();
+
+ private RoleEntry(NodeRole role)
+ {
+ this.role = role;
+ }
+
+ private void register(DiscoveryDruidNode node)
+ {
+ List<DruidNodeDiscovery.Listener> targets;
+ synchronized (LocalDruidNodeDiscoveryProvider.this) {
Review comment:
Seems like `nodes` is not properly synchronized; here the monitor is `LocalDruidNodeDiscoveryProvider.this` but in other methods it's `RoleEntry.this` by way of the method-level `synchronized` keyword.
##########
File path: server/src/main/java/org/apache/druid/discovery/LocalDruidNodeDiscoveryProvider.java
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.discovery;
+
+import org.apache.druid.server.DruidNode;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.BooleanSupplier;
+
+/**
+ * Local node discovery. In this mode, Druid may run a number of
+ * logical servers within a single process, so that node discovery
+ * is all within the same process, but matches a true distributed
+ * node discovery.
+ * <p>
+ * Concurrency is at the level of the entire provider, which is
+ * necessary to coordinate the active state with ongoing node
+ * discovery and listener registrations. Such a "global" lock
+ * is fine because the actions don't occur frequently, nor do the
+ * actions take much time.
+ */
+public class LocalDruidNodeDiscoveryProvider extends DruidNodeDiscoveryProvider implements DruidNodeAnnouncer
+{
+ private class RoleEntry implements DruidNodeDiscovery
+ {
+ @SuppressWarnings("unused")
+ private final NodeRole role;
+ private final Map<DruidNode, DiscoveryDruidNode> nodes = new HashMap<>();
Review comment:
I'm not 100% sure if this is valid. I think it's possible for there to be more than one DiscoveryDruidNode per DruidNode, if a node has more than one NodeRole. Although I haven't dug into this to be sure.
##########
File path: processing/src/main/java/org/apache/druid/query/metadata/metadata/NoneColumnIncluderator.java
##########
@@ -21,6 +21,8 @@
/**
*/
+// Needed for IntelliJ checks
+@SuppressWarnings("unused")
Review comment:
Why wasn't this needed before?
##########
File path: server/src/test/java/org/apache/druid/server/NodeTest.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNull;
+
+public class NodeTest
Review comment:
Would also be good to have an equals verification test here. Like this:
```java
@Test
public void testEqualsAndHashCode()
{
EqualsVerifier.forClass(Node.class)
.usingGetClass()
.verify();
}
```
##########
File path: server/src/main/java/org/apache/druid/server/Node.java
##########
@@ -0,0 +1,99 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Objects;
+
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class Node
Review comment:
This looks a lot like a cut-down DruidNode. To avoid confusion, the javadocs should describe the difference between this and DruidNode, and when we use one vs. the other.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org