vrajat commented on code in PR #15515: URL: https://github.com/apache/pinot/pull/15515#discussion_r2041470757
########## pinot-controller/src/test/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResourceTest.java: ########## @@ -0,0 +1,308 @@ +/** + * 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 java.io.IOException; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.pinot.common.utils.http.HttpClient; +import org.apache.pinot.controller.helix.ControllerRequestClient; +import org.apache.pinot.controller.helix.ControllerTest; +import org.apache.pinot.core.realtime.impl.fakestream.FakeStreamConfigUtils; +import org.apache.pinot.spi.config.table.TableConfig; +import org.apache.pinot.spi.config.table.TableType; +import org.apache.pinot.spi.data.LogicalTable; +import org.apache.pinot.spi.utils.CommonConstants; +import org.apache.pinot.spi.utils.builder.ControllerRequestURLBuilder; +import org.apache.pinot.spi.utils.builder.TableConfigBuilder; +import org.testng.annotations.AfterClass; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; +import static org.testng.Assert.fail; + + +public class PinotLogicalTableResourceTest extends ControllerTest { + + private static final String LOGICAL_TABLE_NAME = "test_logical_table"; + public static final String BROKER_TENANT = "DefaultTenant"; + private static final List<String> PHYSICAL_TABLE_NAMES = List.of("test_table_1", "test_table_2"); + private static final List<String> PHYSICAL_TABLE_NAMES_WITH_TYPE = List.of( + "test_table_1_OFFLINE", + "test_table_1_REALTIME", + "test_table_2_OFFLINE" + ); + protected ControllerRequestURLBuilder _controllerRequestURLBuilder; + + @BeforeClass Review Comment: In the latest version of `PinotSchemaRestletResourceTest`, the setup method is: ``` @BeforeClass public void setUp() throws Exception { TEST_INSTANCE.setupSharedStateAndValidate(); } ``` Can you check if that can be used here as well ? If yes, it will simplify the code ########## pinot-controller/src/test/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResourceTest.java: ########## @@ -0,0 +1,308 @@ +/** + * 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 java.io.IOException; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.pinot.common.utils.http.HttpClient; +import org.apache.pinot.controller.helix.ControllerRequestClient; +import org.apache.pinot.controller.helix.ControllerTest; +import org.apache.pinot.core.realtime.impl.fakestream.FakeStreamConfigUtils; +import org.apache.pinot.spi.config.table.TableConfig; +import org.apache.pinot.spi.config.table.TableType; +import org.apache.pinot.spi.data.LogicalTable; +import org.apache.pinot.spi.utils.CommonConstants; +import org.apache.pinot.spi.utils.builder.ControllerRequestURLBuilder; +import org.apache.pinot.spi.utils.builder.TableConfigBuilder; +import org.testng.annotations.AfterClass; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; +import static org.testng.Assert.fail; + + +public class PinotLogicalTableResourceTest extends ControllerTest { + + private static final String LOGICAL_TABLE_NAME = "test_logical_table"; + public static final String BROKER_TENANT = "DefaultTenant"; + private static final List<String> PHYSICAL_TABLE_NAMES = List.of("test_table_1", "test_table_2"); + private static final List<String> PHYSICAL_TABLE_NAMES_WITH_TYPE = List.of( + "test_table_1_OFFLINE", + "test_table_1_REALTIME", + "test_table_2_OFFLINE" + ); + protected ControllerRequestURLBuilder _controllerRequestURLBuilder; + + @BeforeClass + public void setUpClass() + throws Exception { + startZk(); + startController(); + addFakeBrokerInstancesToAutoJoinHelixCluster(1, true); + addFakeServerInstancesToAutoJoinHelixCluster(1, true); + _controllerRequestURLBuilder = getControllerRequestURLBuilder(); + } + + @AfterClass + public void tearDownClass() { + stopController(); + stopZk(); + } + + @BeforeMethod + public void setUp() + throws Exception { + for (String physicalTable : PHYSICAL_TABLE_NAMES) { + addDummySchema(physicalTable); + addTableConfig(getOfflineTable(physicalTable)); + addTableConfig(getRealtimeTable(physicalTable)); + } + } + + @AfterMethod + public void tearDown() { + cleanup(); + String deleteLogicalTableUrl = _controllerRequestURLBuilder.forLogicalTableDelete(LOGICAL_TABLE_NAME); + try { + ControllerTest.sendDeleteRequest(deleteLogicalTableUrl, getHeaders()); + } catch (IOException e) { + // Ignore exception + } + } + + @Test + public void testCreateUpdateLogicalTable() + throws IOException { + LogicalTable logicalTable = getLogicalTable(LOGICAL_TABLE_NAME, BROKER_TENANT, PHYSICAL_TABLE_NAMES_WITH_TYPE); + + // Add the logical table + String addLogicalTableUrl = _controllerRequestURLBuilder.forLogicalTableCreate(); + String resp = + ControllerTest.sendPostRequest(addLogicalTableUrl, logicalTable.toSingleLineJsonString(), getHeaders()); + assertEquals(resp, + "{\"unrecognizedProperties\":{},\"status\":\"test_logical_table logical table successfully added.\"}"); + + // Retry creating the same logical table + try { + ControllerTest.sendPostRequest(addLogicalTableUrl, logicalTable.toSingleLineJsonString(), getHeaders()); + fail("Logical Table POST request should have failed"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("Logical table: test_logical_table already exists")); + } + + // Get the logical table and verify table config + String getLogicalTableUrl = _controllerRequestURLBuilder.forLogicalTableGet(logicalTable.getTableName()); + LogicalTable remoteLogicalTable = + LogicalTable.fromString(ControllerTest.sendGetRequest(getLogicalTableUrl, getHeaders())); + assertEquals(remoteLogicalTable, logicalTable); + + // Update physical table names and verify + logicalTable.setPhysicalTableNames(List.of( + "test_table_1_OFFLINE", + "test_table_1_REALTIME", + "test_table_2_OFFLINE", + "test_table_2_REALTIME" + )); + String updateLogicalTableUrl = _controllerRequestURLBuilder.forLogicalTableUpdate(logicalTable.getTableName()); + String response = + ControllerTest.sendPutRequest(updateLogicalTableUrl, logicalTable.toSingleLineJsonString(), getHeaders()); + assertEquals(response, + "{\"unrecognizedProperties\":{},\"status\":\"test_logical_table logical table successfully updated.\"}"); + + // Get the logical table and verify updated table config + remoteLogicalTable = LogicalTable.fromString(ControllerTest.sendGetRequest(getLogicalTableUrl, getHeaders())); + assertEquals(remoteLogicalTable, logicalTable); + + // Update logical table with invalid physical table names and verify + logicalTable.setPhysicalTableNames(List.of( + "test_table_1_OFFLINE", + "test_table_2_OFFLINE", + "test_invalid_table_REALTIME" + )); + try { + ControllerTest.sendPutRequest(updateLogicalTableUrl, logicalTable.toSingleLineJsonString(), getHeaders()); + fail("Logical Table POST request should have failed"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("'test_invalid_table_REALTIME' should be one of the existing tables")); + } + } + + @Test + public void testCreateDeleteLogicalTable() + throws IOException { + LogicalTable logicalTable = getLogicalTable(LOGICAL_TABLE_NAME, BROKER_TENANT, PHYSICAL_TABLE_NAMES_WITH_TYPE); + + // Add the logical table + String addLogicalTableUrl = _controllerRequestURLBuilder.forLogicalTableCreate(); + String resp = + ControllerTest.sendPostRequest(addLogicalTableUrl, logicalTable.toSingleLineJsonString(), getHeaders()); + assertEquals(resp, + "{\"unrecognizedProperties\":{},\"status\":\"test_logical_table logical table successfully added.\"}"); + + // Delete the logical table + String deleteLogicalTableUrl = _controllerRequestURLBuilder.forLogicalTableDelete(logicalTable.getTableName()); + String response = ControllerTest.sendDeleteRequest(deleteLogicalTableUrl, getHeaders()); + assertEquals(response, "{\"status\":\"test_logical_table logical table successfully deleted.\"}"); + + // Verify that the logical table is deleted + String getLogicalTableUrl = _controllerRequestURLBuilder.forLogicalTableGet(logicalTable.getTableName()); + try { + ControllerTest.sendGetRequest(getLogicalTableUrl, getHeaders()); + fail("Logical Table GET request should have failed"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("Logical table not found")); + } + } + + @Test + public void testLogicalTableValidationTests() { + String addLogicalTableUrl = _controllerRequestURLBuilder.forLogicalTableCreate(); + String getLogicalTableNamesUrl = _controllerRequestURLBuilder.forLogicalTableNamesGet(); + + // Test logical table name with _OFFLINE and _REALTIME is not allowed + LogicalTable logicalTable = + getLogicalTable("testLogicalTable_OFFLINE", BROKER_TENANT, PHYSICAL_TABLE_NAMES_WITH_TYPE); + try { + ControllerTest.sendPostRequest(addLogicalTableUrl, logicalTable.toSingleLineJsonString(), getHeaders()); + fail("Logical Table POST request should have failed"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("Reason: 'tableName' should not end with _OFFLINE or _REALTIME")); + } + + // Test logical table name can not be same as existing physical table name + logicalTable = + getLogicalTable("test_table_1", BROKER_TENANT, PHYSICAL_TABLE_NAMES_WITH_TYPE); + try { + ControllerTest.sendPostRequest(addLogicalTableUrl, logicalTable.toSingleLineJsonString(), getHeaders()); + fail("Logical Table POST request should have failed"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("Table name: test_table_1 already exists")); + } + + // Test empty physical table names is not allowed + logicalTable = + getLogicalTable(LOGICAL_TABLE_NAME, BROKER_TENANT, List.of()); + try { + ControllerTest.sendPostRequest(addLogicalTableUrl, logicalTable.toSingleLineJsonString(), getHeaders()); + fail("Logical Table POST request should have failed"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("'physicalTableNames' should not be null or empty")); + } + + // Test cannot create logical table with same name twice + logicalTable = getLogicalTable(LOGICAL_TABLE_NAME, BROKER_TENANT, PHYSICAL_TABLE_NAMES_WITH_TYPE); + try { + ControllerTest.sendPostRequest(addLogicalTableUrl, logicalTable.toSingleLineJsonString(), getHeaders()); + String tableNames = ControllerTest.sendGetRequest(getLogicalTableNamesUrl, getHeaders()); + assertEquals(tableNames, "[\"test_logical_table\"]"); + // create the same logical table again + ControllerTest.sendPostRequest(addLogicalTableUrl, logicalTable.toSingleLineJsonString(), getHeaders()); + fail("Logical Table POST request should have failed"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("Logical table: test_logical_table already exists")); + } + } + + @Test Review Comment: Can you check if the above tests can be parameterized using DataProviders ? Then all the tests can be run with different combinations of db.name. Also a test with the logical table in a database is also required. So the combinations I can think are: | Logical Table | Physical Tables | +-------------+----------------+ | No DB | No DB | | No DB | Some in DB | | No DB | All in DB | | DB | No DB | | DB | Some in DB | | DB | All in DB | I am not listing physical tables in different DBs as "Some DB" should take care of that. ########## pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResource.java: ########## @@ -0,0 +1,256 @@ +/** + * 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 io.swagger.annotations.Api; +import io.swagger.annotations.ApiKeyAuthDefinition; +import io.swagger.annotations.ApiOperation; +import io.swagger.annotations.ApiParam; +import io.swagger.annotations.ApiResponse; +import io.swagger.annotations.ApiResponses; +import io.swagger.annotations.Authorization; +import io.swagger.annotations.SecurityDefinition; +import io.swagger.annotations.SwaggerDefinition; +import java.util.List; +import java.util.Map; +import javax.inject.Inject; +import javax.ws.rs.Consumes; +import javax.ws.rs.DELETE; +import javax.ws.rs.GET; +import javax.ws.rs.POST; +import javax.ws.rs.PUT; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.Produces; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.HttpHeaders; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; +import org.apache.arrow.util.Preconditions; +import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.tuple.Pair; +import org.apache.pinot.common.exception.TableNotFoundException; +import org.apache.pinot.common.utils.DatabaseUtils; +import org.apache.pinot.controller.api.access.AccessControlFactory; +import org.apache.pinot.controller.api.access.AccessType; +import org.apache.pinot.controller.api.access.Authenticate; +import org.apache.pinot.controller.api.exception.ControllerApplicationException; +import org.apache.pinot.controller.api.exception.TableAlreadyExistsException; +import org.apache.pinot.controller.helix.core.PinotHelixResourceManager; +import org.apache.pinot.core.auth.Actions; +import org.apache.pinot.core.auth.Authorize; +import org.apache.pinot.core.auth.ManualAuthorization; +import org.apache.pinot.core.auth.TargetType; +import org.apache.pinot.spi.data.LogicalTable; +import org.apache.pinot.spi.utils.JsonUtils; +import org.apache.pinot.spi.utils.builder.TableNameBuilder; +import org.glassfish.grizzly.http.server.Request; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.pinot.spi.utils.CommonConstants.DATABASE; +import static org.apache.pinot.spi.utils.CommonConstants.SWAGGER_AUTHORIZATION_KEY; + + +@Api(tags = "LogicalTable", authorizations = { + @Authorization(value = SWAGGER_AUTHORIZATION_KEY), @Authorization(value = DATABASE) +}) +@SwaggerDefinition(securityDefinition = @SecurityDefinition(apiKeyAuthDefinitions = { + @ApiKeyAuthDefinition(name = HttpHeaders.AUTHORIZATION, in = ApiKeyAuthDefinition.ApiKeyLocation.HEADER, + key = SWAGGER_AUTHORIZATION_KEY, + description = "The format of the key is ```\"Basic <token>\" or \"Bearer <token>\"```"), + @ApiKeyAuthDefinition(name = DATABASE, in = ApiKeyAuthDefinition.ApiKeyLocation.HEADER, key = DATABASE, + description = "Database context passed through http header. If no context is provided 'default' " + + "database context will be considered.") +})) +@Path("/") +public class PinotLogicalTableResource { + public static final Logger LOGGER = LoggerFactory.getLogger(PinotLogicalTableResource.class); + + @Inject + PinotHelixResourceManager _pinotHelixResourceManager; + + @Inject + AccessControlFactory _accessControlFactory; + + @GET + @Produces(MediaType.APPLICATION_JSON) + @Path("/logicalTables") + @Authorize(targetType = TargetType.CLUSTER, paramName = "tableName", action = Actions.Cluster.GET_TABLE) + @ApiOperation(value = "List all logical table names", notes = "Lists all logical table names") + public List<String> listLogicalTableNames(@Context HttpHeaders headers) { + return _pinotHelixResourceManager.getAllLogicalTableNames(); + } + + @GET + @Produces(MediaType.APPLICATION_JSON) + @Path("/logicalTables/{tableName}") + @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action = Actions.Table.GET_TABLE_CONFIG) + @ApiOperation(value = "Get a logical table", notes = "Gets a logical table by name") + @ApiResponses(value = { + @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 404, message = "Logical table not found"), + @ApiResponse(code = 500, message = "Internal error") + }) + public String getLogicalTable( + @ApiParam(value = "Logical table name", required = true) @PathParam("tableName") String tableName, + @Context HttpHeaders headers) { + tableName = DatabaseUtils.translateTableName(tableName, headers); + LOGGER.info("Looking for logical table {}", tableName); + LogicalTable logicalTable = _pinotHelixResourceManager.getLogicalTable(tableName); + if (logicalTable == null) { + throw new ControllerApplicationException(LOGGER, "Logical table not found", Response.Status.NOT_FOUND); + } + return logicalTable.toPrettyJsonString(); + } + + @POST + @Produces(MediaType.APPLICATION_JSON) + @Path("/logicalTables") + @ApiOperation(value = "Add a new logical table", notes = "Adds a new logical table") + @ApiResponses(value = { + @ApiResponse(code = 200, message = "Successfully created logical table"), @ApiResponse(code = 409, message = + "Logical table already exists"), @ApiResponse(code = 400, message = "Missing or invalid request body"), + @ApiResponse(code = 500, message = "Internal error") + }) + @ManualAuthorization + public SuccessResponse addLogicalTable( + String logicalTableJsonString, @Context HttpHeaders httpHeaders, + @Context Request request) { + Pair<LogicalTable, Map<String, Object>> logicalTableAndUnrecognizedProps = + getLogicalAndUnrecognizedPropertiesFromJson(logicalTableJsonString); + LogicalTable logicalTable = logicalTableAndUnrecognizedProps.getLeft(); + + validateLogicalTableName(logicalTable); + String tableName = DatabaseUtils.translateTableName(logicalTable.getTableName(), httpHeaders); + logicalTable.setTableName(tableName); + + // validate permission + ResourceUtils.checkPermissionAndAccess(tableName, request, httpHeaders, AccessType.CREATE, + Actions.Table.CREATE_TABLE, _accessControlFactory, LOGGER); + + SuccessResponse successResponse = addLogicalTable(logicalTable); + return new ConfigSuccessResponse(successResponse.getStatus(), logicalTableAndUnrecognizedProps.getRight()); + } + + @PUT + @Produces(MediaType.APPLICATION_JSON) + @Consumes(MediaType.APPLICATION_JSON) + @Path("/logicalTables/{tableName}") + @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action = Actions.Table.UPDATE_TABLE_CONFIG) + @Authenticate(AccessType.UPDATE) + @ApiOperation(value = "Update a logical table", notes = "Updates a logical table") + @ApiResponses(value = { + @ApiResponse(code = 200, message = "Successfully updated schema"), @ApiResponse(code = 404, message = "Schema " + + "not found"), @ApiResponse(code = 400, message = "Missing or invalid request body"), @ApiResponse(code = 500, + message = "Internal error") + }) + public SuccessResponse updateLogicalTable( + @ApiParam(value = "Name of the logical table", required = true) @PathParam("tableName") String tableName, + @Context HttpHeaders headers, String logicalTableJsonString) { + Pair<LogicalTable, Map<String, Object>> logicalTableAndUnrecognizedProps = + getLogicalAndUnrecognizedPropertiesFromJson(logicalTableJsonString); + LogicalTable logicalTable = logicalTableAndUnrecognizedProps.getLeft(); + + validateLogicalTableName(logicalTable); + Preconditions.checkArgument(logicalTable.getTableName().equals(tableName), + "Logical table name in the request body should match the table name in the URL"); + + tableName = DatabaseUtils.translateTableName(tableName, headers); + logicalTable.setTableName(tableName); + + SuccessResponse successResponse = updateLogicalTable(tableName, logicalTable); + return new ConfigSuccessResponse(successResponse.getStatus(), logicalTableAndUnrecognizedProps.getRight()); + } + + @DELETE + @Produces(MediaType.APPLICATION_JSON) + @Path("/logicalTables/{tableName}") + @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action = Actions.Table.DELETE_TABLE) + @Authenticate(AccessType.DELETE) + @ApiOperation(value = "Delete a logical table", notes = "Deletes a logical table by name") + @ApiResponses(value = { + @ApiResponse(code = 200, message = "Successfully deleted logical table"), @ApiResponse(code = 404, message = + "Logical table not found"), @ApiResponse(code = 500, message = "Error deleting logical table") + }) + public SuccessResponse deleteLogicalTable( + @ApiParam(value = "Logical table name", required = true) @PathParam("tableName") String tableName, + @Context HttpHeaders headers) { + tableName = DatabaseUtils.translateTableName(tableName, headers); + if (_pinotHelixResourceManager.deleteLogicalTable(tableName)) { + return new SuccessResponse(tableName + " logical table successfully deleted."); + } else { + throw new ControllerApplicationException(LOGGER, "Failed to delete logical table", + Response.Status.INTERNAL_SERVER_ERROR); + } + } + + private void validateLogicalTableName(LogicalTable logicalTable) { Review Comment: This method and `PinotHelixResourceManager.validateLogicalTable` can be merged. I prefer merging the other function to this one. ########## pinot-controller/src/test/java/org/apache/pinot/controller/api/resources/PinotWithAuthLogicalTableResourceTest.java: ########## @@ -0,0 +1,48 @@ +/** + * 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 java.util.Map; +import org.apache.pinot.controller.helix.ControllerRequestClient; + + +public class PinotWithAuthLogicalTableResourceTest extends PinotLogicalTableResourceTest { Review Comment: I think more setup is required. IIUC the auth headers are ignored. Controller has to be started with auth configuration. Check `CursorWithAuthIntegrationTest` and `BasicAuthTestUtils` esp. `addControllerConfiguration` ########## pinot-spi/src/main/java/org/apache/pinot/spi/data/LogicalTable.java: ########## @@ -0,0 +1,126 @@ +/** + * 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.spi.data; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.ObjectNode; +import java.io.File; +import java.io.IOException; +import java.util.List; +import java.util.Objects; +import org.apache.pinot.spi.utils.JsonUtils; + + +@JsonIgnoreProperties(ignoreUnknown = true) +public class LogicalTable { + + public static final String TABLE_NAME_KEY = "tableName"; + public static final String PHYSICAL_TABLE_NAMES_KEY = "physicalTableNames"; + public static final String BROKER_TENANT_KEY = "brokerTenant"; Review Comment: Can you remove broker tenant ? Based on your q. and offline discussion, we should investigate this feature more thoroughly. ########## pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableUtils.java: ########## @@ -0,0 +1,66 @@ +/** + * 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.common.utils; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import java.io.IOException; +import javax.annotation.Nonnull; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.pinot.spi.data.LogicalTable; +import org.apache.pinot.spi.utils.JsonUtils; + + +/** + * Utility class which contains {@link LogicalTable} related operations. + */ +public class LogicalTableUtils { Review Comment: I had created this class as I was following the code for `Schema` and that had a `SchemaUtils` class. For logical tables, these functions are only used in `ZkMetadataProvider`. I dont see the value of keeping these functions in a separate class. Can you move these methods to `ZkMetadataProvider` and name them appropriately ? `getLogicalTableFromZNRecord` and `getZNRecordFromLogicalTable` ? -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
