Jackie-Jiang commented on a change in pull request #7556:
URL: https://github.com/apache/pinot/pull/7556#discussion_r727502324
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
##########
@@ -68,7 +69,8 @@ public String getTableInstances(
ObjectNode e = JsonUtils.newObjectNode();
e.put("tableType", "offline");
ArrayNode a = JsonUtils.newArrayNode();
- for (String ins :
_pinotHelixResourceManager.getBrokerInstancesForTable(tableName,
TableType.OFFLINE)) {
+ for (String ins : _pinotHelixResourceManager
+
.getLiveBrokersForTable(TableNameBuilder.OFFLINE.tableNameWithType(tableName)))
{
Review comment:
This will make the broker and server behavior inconsistent (currently
both of them are tag based), and change the behavior of this API
##########
File path: pinot-controller/pom.xml
##########
@@ -197,6 +197,10 @@
<groupId>org.quartz-scheduler</groupId>
<artifactId>quartz</artifactId>
</dependency>
+ <dependency>
Review comment:
Pinot-controller should not include client module. We should implement
it separately
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
##########
@@ -113,4 +114,45 @@ public String getTableInstances(
ret.set("server", servers); // Keeping compatibility with previous API,
so "server" and "brokers"
return ret.toString();
}
+
+ @GET
+ @Path("/tables/{tableName}/brokers")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List the brokers serving a table", notes = "List live
brokers of the given table based on EV")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ public String getTableBrokers(
Review comment:
```suggestion
public String getTableLiveBrokers(
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/BrokerResourceExternalViewReader.java
##########
@@ -0,0 +1,135 @@
+/**
+ * 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.pinot.controller.helix.core.util;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.zip.GZIPInputStream;
+import org.I0Itec.zkclient.ZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Reads brokers external view from Zookeeper
+ */
+public class BrokerResourceExternalViewReader {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(BrokerResourceExternalViewReader.class);
+ private static final ObjectReader OBJECT_READER = new
ObjectMapper().reader();
+ public static final String BROKER_EXTERNAL_VIEW_PATH =
"/EXTERNALVIEW/brokerResource";
+ public static final String REALTIME_SUFFIX = "_REALTIME";
+ public static final String OFFLINE_SUFFIX = "_OFFLINE";
+
+ private ZkClient _zkClient;
+
+ public BrokerResourceExternalViewReader(ZkClient zkClient) {
Review comment:
Move the logic in this class into the `PinotHelixResourceManager`. We
don't need to use the `ZkClient` api to read the external view
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
##########
@@ -113,4 +114,45 @@ public String getTableInstances(
ret.set("server", servers); // Keeping compatibility with previous API,
so "server" and "brokers"
return ret.toString();
}
+
+ @GET
+ @Path("/tables/{tableName}/brokers")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List the brokers serving a table", notes = "List live
brokers of the given table based on EV")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ public String getTableBrokers(
+ @ApiParam(value = "Table name without type", required = true)
@PathParam("tableName") String tableName) {
+ ObjectNode ret = JsonUtils.newObjectNode();
Review comment:
Let's not reuse the response format for this new API. Would suggest just
returning an array of live brokers.
- If the input is a table name with type, returns the live brokers for the
table, or 404 if table is not found
- If the input is a raw table name, if both offline and realtime table
exist, return the brokers live for both tables (we should only route query to
the broker with both tables online); if only one table exist, return the live
brokers for the single table; if no table is found, return 404
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2862,6 +2864,14 @@ public TableStats getTableStats(String
tableNameWithType) {
return new TableStats(creationTime);
}
+ // Return the list of live brokers serving a table. Each entry is of the
following format:
+ // Broker_hostname_port (e.g., broker_12.34.56.78_1234)
+ public List<String> getLiveBrokersForTable(String tableNameWithType) {
+ BrokerResourceExternalViewReader externalViewReader =
Review comment:
This can be simplified as you have access to helix property in this
class. You can get the `brokerResource` external view by calling
`_helixDataAccessor.getProperty(_keyBuilder.externalView(CommonConstants.Helix.BROKER_RESOURCE_INSTANCE))`
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
##########
@@ -113,4 +114,45 @@ public String getTableInstances(
ret.set("server", servers); // Keeping compatibility with previous API,
so "server" and "brokers"
return ret.toString();
}
+
+ @GET
+ @Path("/tables/{tableName}/brokers")
Review comment:
Suggest changing it to `/tables/{tableName}/livebrokers` to
differentiate it from the other one
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2862,6 +2864,14 @@ public TableStats getTableStats(String
tableNameWithType) {
return new TableStats(creationTime);
}
+ // Return the list of live brokers serving a table. Each entry is of the
following format:
+ // Broker_hostname_port (e.g., broker_12.34.56.78_1234)
+ public List<String> getLiveBrokersForTable(String tableNameWithType) {
+ BrokerResourceExternalViewReader externalViewReader =
Review comment:
The table existence check can be pushed here so that we only read the
external view once
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/BrokerResourceExternalViewReader.java
##########
@@ -0,0 +1,135 @@
+/**
+ * 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.pinot.controller.helix.core.util;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.zip.GZIPInputStream;
+import org.I0Itec.zkclient.ZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Reads brokers external view from Zookeeper
+ */
+public class BrokerResourceExternalViewReader {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(BrokerResourceExternalViewReader.class);
+ private static final ObjectReader OBJECT_READER = new
ObjectMapper().reader();
+ public static final String BROKER_EXTERNAL_VIEW_PATH =
"/EXTERNALVIEW/brokerResource";
+ public static final String REALTIME_SUFFIX = "_REALTIME";
+ public static final String OFFLINE_SUFFIX = "_OFFLINE";
+
+ private ZkClient _zkClient;
+
+ public BrokerResourceExternalViewReader(ZkClient zkClient) {
+ _zkClient = zkClient;
+ }
+
+ protected ByteArrayInputStream getInputStream(byte[] brokerResourceNodeData)
{
+ return new ByteArrayInputStream(brokerResourceNodeData);
+ }
+
+ // Return a map from tablename (without type) to live brokers (of format
host:port).
+ public Map<String, List<String>> getTableToBrokersMap() {
+ return getTableToBrokersMapFromExternalView(false, true);
+ }
+
+ // Return a map from tablename (with type) to live brokers (of raw instance
format broker_host_port).
+ public Map<String, List<String>> getTableWithTypeToRawBrokerInstanceIdsMap()
{
+ return getTableToBrokersMapFromExternalView(true, false);
+ }
+
+ private Map<String, List<String>>
getTableToBrokersMapFromExternalView(boolean tableWithType, boolean
useUrlFormat) {
+ Map<String, Set<String>> brokerUrlsMap = new HashMap<>();
+ try {
+ byte[] brokerResourceNodeData =
_zkClient.readData(BROKER_EXTERNAL_VIEW_PATH, true);
+ brokerResourceNodeData = unpackZnodeIfNecessary(brokerResourceNodeData);
+ JsonNode jsonObject =
OBJECT_READER.readTree(getInputStream(brokerResourceNodeData));
+ JsonNode brokerResourceNode = jsonObject.get("mapFields");
+
+ Iterator<Map.Entry<String, JsonNode>> resourceEntries =
brokerResourceNode.fields();
+ while (resourceEntries.hasNext()) {
+ Map.Entry<String, JsonNode> resourceEntry = resourceEntries.next();
+ String resourceName = resourceEntry.getKey();
+ String tableName =
+ tableWithType ? resourceName :
resourceName.replace(OFFLINE_SUFFIX, "").replace(REALTIME_SUFFIX, "");
+ Set<String> brokerUrls = brokerUrlsMap.computeIfAbsent(tableName, k ->
new HashSet<>());
+ JsonNode resource = resourceEntry.getValue();
+ Iterator<Map.Entry<String, JsonNode>> brokerEntries =
resource.fields();
+ while (brokerEntries.hasNext()) {
+ Map.Entry<String, JsonNode> brokerEntry = brokerEntries.next();
+ String brokerName = brokerEntry.getKey();
+ if (brokerName.startsWith("Broker_") &&
"ONLINE".equals(brokerEntry.getValue().asText())) {
Review comment:
The prefix check should be removed as it won't work if the broker name
is customized
--
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]
