vrajat commented on code in PR #15720:
URL: https://github.com/apache/pinot/pull/15720#discussion_r2078952919
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResourceTest.java:
##########
@@ -129,90 +133,191 @@ public void testCreateUpdateDeleteLogicalTables(String
logicalTableName, List<St
}
@Test
- public void testLogicalTableValidationTests()
+ public void testLogicalTableQuoteConfigValidation()
throws IOException {
- String addLogicalTableUrl =
_controllerRequestURLBuilder.forLogicalTableCreate();
+ List<String> physicalTableNamesWithType =
createHybridTables(List.of("test_table_1"));
+ LogicalTableConfig logicalTableConfig =
+ getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setQuotaConfig(new QuotaConfig("10G", "999"));
+ try {
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ fail("Logical Table POST request should have failed");
+ } catch (IOException e) {
+ assertTrue(e.getMessage().contains("Reason: 'quota.storage' should not
be set for logical table"),
+ e.getMessage());
+ }
Review Comment:
Can you also get and verify that the match ? Basically test the whole round
trip
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResourceTest.java:
##########
@@ -129,90 +133,191 @@ public void testCreateUpdateDeleteLogicalTables(String
logicalTableName, List<St
}
@Test
- public void testLogicalTableValidationTests()
+ public void testLogicalTableQuoteConfigValidation()
throws IOException {
- String addLogicalTableUrl =
_controllerRequestURLBuilder.forLogicalTableCreate();
+ List<String> physicalTableNamesWithType =
createHybridTables(List.of("test_table_1"));
+ LogicalTableConfig logicalTableConfig =
+ getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setQuotaConfig(new QuotaConfig("10G", "999"));
+ try {
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ fail("Logical Table POST request should have failed");
+ } catch (IOException e) {
+ assertTrue(e.getMessage().contains("Reason: 'quota.storage' should not
be set for logical table"),
+ e.getMessage());
+ }
+ }
- // create physical tables
- List<String> physicalTableNames = List.of("test_table_1", "test_table_2");
- List<String> physicalTableNamesWithType =
createHybridTables(physicalTableNames);
+ @Test
+ public void testLogicalTableReferenceTableValidation()
+ throws IOException {
+ List<String> physicalTableNamesWithType =
createHybridTables(List.of("test_table_1"));
- // Test logical table name with _OFFLINE and _REALTIME is not allowed
+ // Test ref offline table name is null validation
LogicalTableConfig logicalTableConfig =
- getDummyLogicalTableConfig("testLogicalTable_OFFLINE",
physicalTableNamesWithType, BROKER_TENANT);
+ getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setRefOfflineTableName(null);
try {
- ControllerTest.sendPostRequest(addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.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"),
+ assertTrue(e.getMessage()
+ .contains("Reason: 'refOfflineTableName' should not be null or
empty when offline table exists"),
e.getMessage());
}
- logicalTableConfig =
- getDummyLogicalTableConfig("testLogicalTable_REALTIME",
physicalTableNamesWithType, BROKER_TENANT);
+ // Test ref realtime table name is null validation
+ logicalTableConfig = getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setRefRealtimeTableName(null);
try {
- ControllerTest.sendPostRequest(addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.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"),
+ assertTrue(e.getMessage()
+ .contains("Reason: 'refRealtimeTableName' should not be null or
empty when realtime table exists"),
e.getMessage());
}
- // Test logical table name can not be same as existing physical table name
- logicalTableConfig =
- getDummyLogicalTableConfig("test_table_1", physicalTableNamesWithType,
BROKER_TENANT);
+ // Test ref offline table is present in the offline tables validation
+ logicalTableConfig = getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setRefOfflineTableName("random_table_OFFLINE");
try {
- ControllerTest.sendPostRequest(addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
fail("Logical Table POST request should have failed");
} catch (IOException e) {
- assertTrue(e.getMessage().contains("Table name: test_table_1 already
exists"), e.getMessage());
+ assertTrue(e.getMessage().contains("Reason: 'refOfflineTableName' should
be one of the provided offline tables"),
+ e.getMessage());
}
+ // Test ref realtime table is present in the realtime tables validation
+ logicalTableConfig = getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setRefRealtimeTableName("random_table_REALTIME");
+ try {
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ fail("Logical Table POST request should have failed");
+ } catch (IOException e) {
+ assertTrue(
+ e.getMessage().contains("Reason: 'refRealtimeTableName' should be
one of the provided realtime tables"),
+ e.getMessage());
+ }
+ }
+
+ @Test
+ public void testLogicalTableBrokerTenantValidation()
+ throws IOException {
+ List<String> physicalTableNamesWithType =
createHybridTables(List.of("test_table_1"));
+ LogicalTableConfig logicalTableConfig =
+ getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, "InvalidTenant");
+ try {
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ fail("Logical Table POST request should have failed");
+ } catch (IOException e) {
+ assertTrue(e.getMessage().contains("Reason: 'InvalidTenant' should be
one of the existing broker tenants"),
Review Comment:
`expectedException` and `excepectedExceptionErrorMessageRegexp` is preferred
if this only ONE negative test.
https://howtodoinjava.com/testng/testng-expected-exception/
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResourceTest.java:
##########
@@ -129,90 +133,191 @@ public void testCreateUpdateDeleteLogicalTables(String
logicalTableName, List<St
}
@Test
- public void testLogicalTableValidationTests()
+ public void testLogicalTableQuoteConfigValidation()
Review Comment:
Typo: `Quota` vs `Quote`
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResourceTest.java:
##########
@@ -129,90 +133,191 @@ public void testCreateUpdateDeleteLogicalTables(String
logicalTableName, List<St
}
@Test
- public void testLogicalTableValidationTests()
+ public void testLogicalTableQuoteConfigValidation()
throws IOException {
- String addLogicalTableUrl =
_controllerRequestURLBuilder.forLogicalTableCreate();
+ List<String> physicalTableNamesWithType =
createHybridTables(List.of("test_table_1"));
+ LogicalTableConfig logicalTableConfig =
+ getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setQuotaConfig(new QuotaConfig("10G", "999"));
+ try {
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ fail("Logical Table POST request should have failed");
+ } catch (IOException e) {
+ assertTrue(e.getMessage().contains("Reason: 'quota.storage' should not
be set for logical table"),
+ e.getMessage());
+ }
+ }
- // create physical tables
- List<String> physicalTableNames = List.of("test_table_1", "test_table_2");
- List<String> physicalTableNamesWithType =
createHybridTables(physicalTableNames);
+ @Test
+ public void testLogicalTableReferenceTableValidation()
+ throws IOException {
+ List<String> physicalTableNamesWithType =
createHybridTables(List.of("test_table_1"));
- // Test logical table name with _OFFLINE and _REALTIME is not allowed
+ // Test ref offline table name is null validation
LogicalTableConfig logicalTableConfig =
- getDummyLogicalTableConfig("testLogicalTable_OFFLINE",
physicalTableNamesWithType, BROKER_TENANT);
+ getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setRefOfflineTableName(null);
try {
- ControllerTest.sendPostRequest(addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.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"),
+ assertTrue(e.getMessage()
+ .contains("Reason: 'refOfflineTableName' should not be null or
empty when offline table exists"),
e.getMessage());
}
- logicalTableConfig =
- getDummyLogicalTableConfig("testLogicalTable_REALTIME",
physicalTableNamesWithType, BROKER_TENANT);
+ // Test ref realtime table name is null validation
+ logicalTableConfig = getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setRefRealtimeTableName(null);
try {
- ControllerTest.sendPostRequest(addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.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"),
+ assertTrue(e.getMessage()
+ .contains("Reason: 'refRealtimeTableName' should not be null or
empty when realtime table exists"),
e.getMessage());
}
- // Test logical table name can not be same as existing physical table name
- logicalTableConfig =
- getDummyLogicalTableConfig("test_table_1", physicalTableNamesWithType,
BROKER_TENANT);
+ // Test ref offline table is present in the offline tables validation
+ logicalTableConfig = getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setRefOfflineTableName("random_table_OFFLINE");
try {
- ControllerTest.sendPostRequest(addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
fail("Logical Table POST request should have failed");
} catch (IOException e) {
- assertTrue(e.getMessage().contains("Table name: test_table_1 already
exists"), e.getMessage());
+ assertTrue(e.getMessage().contains("Reason: 'refOfflineTableName' should
be one of the provided offline tables"),
+ e.getMessage());
}
+ // Test ref realtime table is present in the realtime tables validation
+ logicalTableConfig = getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, BROKER_TENANT);
+ logicalTableConfig.setRefRealtimeTableName("random_table_REALTIME");
+ try {
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ fail("Logical Table POST request should have failed");
+ } catch (IOException e) {
+ assertTrue(
+ e.getMessage().contains("Reason: 'refRealtimeTableName' should be
one of the provided realtime tables"),
+ e.getMessage());
+ }
+ }
+
+ @Test
+ public void testLogicalTableBrokerTenantValidation()
+ throws IOException {
+ List<String> physicalTableNamesWithType =
createHybridTables(List.of("test_table_1"));
+ LogicalTableConfig logicalTableConfig =
+ getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, "InvalidTenant");
+ try {
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ fail("Logical Table POST request should have failed");
+ } catch (IOException e) {
+ assertTrue(e.getMessage().contains("Reason: 'InvalidTenant' should be
one of the existing broker tenants"),
+ e.getMessage());
+ }
+ }
+
+ @Test
+ public void testLogicalTablePhysicalTableConfigValidation() {
// Test empty physical table names is not allowed
- logicalTableConfig =
+ LogicalTableConfig logicalTableConfig =
getDummyLogicalTableConfig(LOGICAL_TABLE_NAME, List.of(),
BROKER_TENANT);
try {
- ControllerTest.sendPostRequest(addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
fail("Logical Table POST request should have failed");
} catch (IOException e) {
assertTrue(e.getMessage().contains("'physicalTableConfigMap' should not
be null or empty"), e.getMessage());
}
// Test all table names are physical table names and none is hybrid table
name
+ List<String> physicalTableNames = List.of("test_table_1");
logicalTableConfig = getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNames, BROKER_TENANT);
try {
- ControllerTest.sendPostRequest(addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
fail("Logical Table POST request should have failed");
} catch (IOException e) {
assertTrue(e.getMessage().contains("Reason: 'test_table_1' should be one
of the existing tables"),
e.getMessage());
}
+ }
- // Test valid broker tenant is provided
- logicalTableConfig = getDummyLogicalTableConfig(LOGICAL_TABLE_NAME,
physicalTableNamesWithType, "InvalidTenant");
+ @Test
+ public void testLogicalTableNameCannotSameAsPhysicalTableNameValidation()
+ throws IOException {
+ List<String> physicalTableNamesWithType =
createHybridTables(List.of("test_table_1"));
+ LogicalTableConfig logicalTableConfig =
+ getDummyLogicalTableConfig("test_table_1", physicalTableNamesWithType,
BROKER_TENANT);
try {
- ControllerTest.sendPostRequest(addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.toSingleLineJsonString(), getHeaders());
fail("Logical Table POST request should have failed");
} catch (IOException e) {
- assertTrue(e.getMessage().contains("Reason: 'InvalidTenant' should be
one of the existing broker tenants"),
+ assertTrue(e.getMessage().contains("Table name: test_table_1 already
exists"), e.getMessage());
+ }
+ }
+
+ @Test
+ public void testLogicalTableNameSuffixValidation()
+ throws IOException {
+ List<String> physicalTableNamesWithType =
createHybridTables(List.of("test_table_1"));
+
+ // Test logical table name with _OFFLINE and _REALTIME is not allowed
+ LogicalTableConfig logicalTableConfig =
+ getDummyLogicalTableConfig("testLogicalTable_OFFLINE",
physicalTableNamesWithType, BROKER_TENANT);
+ try {
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.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"),
e.getMessage());
}
+
+ logicalTableConfig =
+ getDummyLogicalTableConfig("testLogicalTable_REALTIME",
physicalTableNamesWithType, BROKER_TENANT);
+ try {
+ ControllerTest.sendPostRequest(_addLogicalTableUrl,
logicalTableConfig.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"),
+ e.getMessage());
+ }
+ }
+
+ @DataProvider
Review Comment:
Can you check if there is already a table provider in `ClusterTest` ?
--
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
