You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/07/11 03:04:09 UTC
[GitHub] [incubator-pinot] fx19880617 opened a new pull request #5685: Adding controller APIs to fetch brokers information
fx19880617 opened a new pull request #5685:
URL: https://github.com/apache/incubator-pinot/pull/5685
## Description
Adding controller APIs to fetch brokers information
Per issue: https://github.com/apache/incubator-pinot/issues/5682
## Release Notes
Adding Controller APIs to fetch brokers information:
```
// Return all the {state} brokers mapping of tenants/tables
GET /brokers?state={online|offline}
// Return all the {state} brokers mapping per tenants
GET /brokers/tenants?state={online|offline}
// Return {state} brokers for a given {tenant}, returns all brokers if state is not specified.
GET /brokers/tenants/{tenant}?state={online|offline}
// Return all the {state} brokers mapping per table
GET /brokers/tenants?state={online|offline}
// Return {state} brokers for a given {table}, returns all brokers if state is not specified.
GET /brokers/tables/{table}?state={online|offline}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 merged pull request #5685: Adding controller APIs to fetch brokers information
Posted by GitBox <gi...@apache.org>.
fx19880617 merged pull request #5685:
URL: https://github.com/apache/incubator-pinot/pull/5685
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5685: Adding controller APIs to fetch brokers information
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5685:
URL: https://github.com/apache/incubator-pinot/pull/5685#issuecomment-657692410
In order to be consistent with other APIs, I would take the raw table name, and a query parameter of table type in the API (instead of tableName_OFFLINE. etc.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #5685: Adding controller APIs to fetch brokers information
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #5685:
URL: https://github.com/apache/incubator-pinot/pull/5685#discussion_r454618296
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotBrokerRestletResource.java
##########
@@ -0,0 +1,146 @@
+/**
+ * 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.api.resources;
+
+import com.google.common.collect.ImmutableList;
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.common.exception.TableNotFoundException;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+@Api(tags = Constants.BROKER_TAG)
+@Path("/")
+public class PinotBrokerRestletResource {
+ public static final Logger LOGGER = LoggerFactory.getLogger(PinotBrokerRestletResource.class);
+ private static final String TYPE_REALTIME = "_REALTIME";
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #5685: Adding controller APIs to fetch brokers information
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5685:
URL: https://github.com/apache/incubator-pinot/pull/5685#discussion_r453778450
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotBrokerRestletResource.java
##########
@@ -0,0 +1,132 @@
+/**
+ * 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.api.resources;
+
+import com.google.common.collect.ImmutableList;
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.MediaType;
+import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
+import org.apache.pinot.spi.config.table.TableType;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+@Api(tags = Constants.BROKER_TAG)
+@Path("/")
+public class PinotBrokerRestletResource {
+ public static final Logger LOGGER = LoggerFactory.getLogger(PinotBrokerRestletResource.class);
+
+ @Inject
+ PinotHelixResourceManager _pinotHelixResourceManager;
+
+ @GET
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("/brokers")
+ @ApiOperation(value = "List tenants and tables to brokers mappings", notes = "List tenants and tables to brokers mappings")
+ public Map<String, Map<String, List<String>>> listBrokersMapping(
+ @ApiParam(value = "ONLINE|OFFLINE") @QueryParam("state") String state) {
+ Map<String, Map<String, List<String>>> resultMap = new HashMap<>();
+ resultMap.put("tenants", getTenantsToBrokersMapping(state));
+ resultMap.put("tables", getTablesToBrokersMapping(state));
+ return resultMap;
+ }
+
+ @GET
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("/brokers/tenants")
+ @ApiOperation(value = "List tenants to brokers mappings", notes = "List tenants to brokers mappings")
+ public Map<String, List<String>> getTenantsToBrokersMapping(
+ @ApiParam(value = "ONLINE|OFFLINE") @QueryParam("state") String state) {
+ Map<String, List<String>> resultMap = new HashMap<>();
+ _pinotHelixResourceManager.getAllBrokerTenantNames().stream()
+ .forEach(tenant -> resultMap.put(tenant, getBrokersForTenant(tenant, state)));
+ return resultMap;
+ }
+
+ @GET
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("/brokers/tenants/{tenantName}")
+ @ApiOperation(value = "List brokers for a given tenant", notes = "List brokers for a given tenant")
+ public List<String> getBrokersForTenant(
+ @ApiParam(value = "Name of the tenant", required = true) @PathParam("tenantName") String tenantName,
+ @ApiParam(value = "ONLINE|OFFLINE") @QueryParam("state") String state) {
+ Set<String> tenantBrokers = new HashSet<>();
+ tenantBrokers.addAll(_pinotHelixResourceManager.getAllInstancesForBrokerTenant(tenantName));
+ applyStateChanges(tenantBrokers, state);
+ return ImmutableList.copyOf(tenantBrokers);
+ }
+
+ @GET
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("/brokers/tables")
+ @ApiOperation(value = "List tables to brokers mappings", notes = "List tables to brokers mappings")
+ public Map<String, List<String>> getTablesToBrokersMapping(
+ @ApiParam(value = "ONLINE|OFFLINE") @QueryParam("state") String state) {
+ Map<String, List<String>> resultMap = new HashMap<>();
+ _pinotHelixResourceManager.getAllTables().stream()
+ .forEach(table -> resultMap.put(table, getBrokersForTable(table, state)));
+ return resultMap;
+ }
+
+ @GET
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("/brokers/tables/{tableName}")
+ @ApiOperation(value = "List brokers for a given table", notes = "List brokers for a given table")
+ public List<String> getBrokersForTable(
+ @ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
+ @ApiParam(value = "ONLINE|OFFLINE") @QueryParam("state") String state) {
+ String actualTableName = _pinotHelixResourceManager.getActualTableName(tableName);
+ Set<String> tableBrokers = new HashSet<>();
+ if (_pinotHelixResourceManager.hasOfflineTable(actualTableName)) {
+ tableBrokers.addAll(_pinotHelixResourceManager.getBrokerInstancesForTable(actualTableName, TableType.OFFLINE));
+ }
+ if (_pinotHelixResourceManager.hasRealtimeTable(actualTableName)) {
+ tableBrokers.addAll(_pinotHelixResourceManager.getBrokerInstancesForTable(actualTableName, TableType.REALTIME));
+ }
+ applyStateChanges(tableBrokers, state);
+ return ImmutableList.copyOf(tableBrokers);
+ }
+
+ private void applyStateChanges(Set<String> brokers, String state) {
+ if (state == null) {
+ return;
+ }
+ switch (state) {
+ case "ONLINE":
Review comment:
use the constant from statemodel
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on pull request #5685: Adding controller APIs to fetch brokers information
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #5685:
URL: https://github.com/apache/incubator-pinot/pull/5685#issuecomment-658120119
> Also, we don't allow admin to specify different broker tenant names for realtime and offline tables. I would combine the two in the output as well. Keeps it consistent with the table config settings.
> Otherwise, you run the risk of an admin trying to add broker tenant as "myTenant_REALTIME" or "myTenant_OFFLINE" in the table config.
Correct, broker tenant has no offline/realtime suffix also changed the behavior for tables to remove suffix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on pull request #5685: Adding controller APIs to fetch brokers information
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #5685:
URL: https://github.com/apache/incubator-pinot/pull/5685#issuecomment-658118785
> In order to be consistent with other APIs, I would take the raw table name, and a query parameter of table type in the API (instead of tableName_OFFLINE. etc.)
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5685: Adding controller APIs to fetch brokers information
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5685:
URL: https://github.com/apache/incubator-pinot/pull/5685#issuecomment-657694285
Also, we don't allow admin to specify different broker tenant names for realtime and offline tables. I would combine the two in the output as well. Keeps it consistent with the table config settings.
Otherwise, you run the risk of an admin trying to add broker tenant as "myTenant_REALTIME" or "myTenant_OFFLINE" in the table config.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5685: Adding controller APIs to fetch brokers information
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5685:
URL: https://github.com/apache/incubator-pinot/pull/5685#discussion_r454613316
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotBrokerRestletResource.java
##########
@@ -0,0 +1,146 @@
+/**
+ * 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.api.resources;
+
+import com.google.common.collect.ImmutableList;
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.common.exception.TableNotFoundException;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+@Api(tags = Constants.BROKER_TAG)
+@Path("/")
+public class PinotBrokerRestletResource {
+ public static final Logger LOGGER = LoggerFactory.getLogger(PinotBrokerRestletResource.class);
+ private static final String TYPE_REALTIME = "_REALTIME";
Review comment:
these two definitions are not used anywhere right?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotBrokerRestletResource.java
##########
@@ -103,28 +110,35 @@
@ApiOperation(value = "List brokers for a given table", notes = "List brokers for a given table")
public List<String> getBrokersForTable(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
+ @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
@ApiParam(value = "ONLINE|OFFLINE") @QueryParam("state") String state) {
- String actualTableName = _pinotHelixResourceManager.getActualTableName(tableName);
- Set<String> tableBrokers = new HashSet<>();
- if (_pinotHelixResourceManager.hasOfflineTable(actualTableName)) {
- tableBrokers.addAll(_pinotHelixResourceManager.getBrokerInstancesForTable(actualTableName, TableType.OFFLINE));
- }
- if (_pinotHelixResourceManager.hasRealtimeTable(actualTableName)) {
- tableBrokers.addAll(_pinotHelixResourceManager.getBrokerInstancesForTable(actualTableName, TableType.REALTIME));
+ try {
+ List<String> tableNamesWithType = _pinotHelixResourceManager
+ .getExistingTableNamesWithType(tableName, Constants.validateTableType(tableTypeStr));
+ if (tableNamesWithType.isEmpty()) {
+ throw new ControllerApplicationException(LOGGER, String.format("Table [%s] not found.", tableName),
Review comment:
nit:
```suggestion
throw new ControllerApplicationException(LOGGER, String.format("Table '%s' not found.", tableName),
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 edited a comment on pull request #5685: Adding controller APIs to fetch brokers information
Posted by GitBox <gi...@apache.org>.
fx19880617 edited a comment on pull request #5685:
URL: https://github.com/apache/incubator-pinot/pull/5685#issuecomment-658120119
> Also, we don't allow admin to specify different broker tenant names for realtime and offline tables. I would combine the two in the output as well. Keeps it consistent with the table config settings.
> Otherwise, you run the risk of an admin trying to add broker tenant as "myTenant_REALTIME" or "myTenant_OFFLINE" in the table config.
Correct, broker tenant has no offline/realtime suffix also changed the behavior for tables to remove suffix.
Updated the PR description also.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org