[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-09-17 Thread GitBox
jackjlli commented on a change in pull request #4608: Unit tests and bug fixes 
for DeleteTable rest API for controller.
URL: https://github.com/apache/incubator-pinot/pull/4608#discussion_r325409807
 
 

 ##
 File path: 
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java
 ##
 @@ -273,6 +275,76 @@ public void rebalanceTableWithoutSegments()
 Assert.assertEquals(rebalanceResult.getStatus(), 
RebalanceResult.Status.NO_OP);
   }
 
+  @Test
+  public void testDeleteTable() throws IOException {
+// Case 1: Create a REALTIME table and delete it directly w/o using query 
param.
+TableConfig realtimeTableConfig = 
_realtimeBuilder.setTableName("table0").build();
+String creationResponse =
+sendPostRequest(_createTableUrl, 
realtimeTableConfig.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table0_REALTIME 
succesfully added\"}");
+
+// Delete realtime table using REALTIME suffix.
+String deleteResponse = sendDeleteRequest(StringUtil.join("/", 
this._controllerBaseApiUrl,
+"tables", "testRealtimeTable_REALTIME"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables: 
[testRealtimeTable_REALTIME] deleted\"}");
+
+// Case 2: Create an offline table and delete it directly w/o using query 
param.
+TableConfig offlineTableConfig = 
_offlineBuilder.setTableName("table0").build();
+creationResponse =
+sendPostRequest(_createTableUrl, 
offlineTableConfig.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table0_OFFLINE 
succesfully added\"}");
+
+// Delete offline table using OFFLINE suffix.
+deleteResponse = sendDeleteRequest(StringUtil.join("/", 
this._controllerBaseApiUrl,
+"tables", "table0_OFFLINE"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables: 
[table0_OFFLINE] deleted\"}");
+
+// Case 3: Create REALTIME and OFFLINE tables and delete both of them.
+TableConfig rtConfig1 = _realtimeBuilder.setTableName("table1").build();
+creationResponse =
+sendPostRequest(_createTableUrl, rtConfig1.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table1_REALTIME 
succesfully added\"}");
+
+TableConfig offlineConfig1 = 
_offlineBuilder.setTableName("table1").build();
+creationResponse =
+sendPostRequest(_createTableUrl, offlineConfig1.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table1_OFFLINE 
succesfully added\"}");
+
+deleteResponse = sendDeleteRequest(StringUtil.join("/", 
this._controllerBaseApiUrl,
+"tables", "table1"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables: 
[table1_OFFLINE, table1_REALTIME] deleted\"}");
+
+
+// Case 4: Create REALTIME and OFFLINE tables and delete the 
realtime/offline table using query params.
+TableConfig rtConfig2 = _realtimeBuilder.setTableName("table2").build();
+creationResponse =
+sendPostRequest(_createTableUrl, rtConfig2.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table2_REALTIME 
succesfully added\"}");
+
+TableConfig offlineConfig2 = 
_offlineBuilder.setTableName("table2").build();
+creationResponse =
+sendPostRequest(_createTableUrl, offlineConfig2.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table2_OFFLINE 
succesfully added\"}");
+
+deleteResponse = sendDeleteRequest(StringUtil.join("/", 
this._controllerBaseApiUrl,
+"tables", "table2?type=realtime"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables: 
[table2_REALTIME] deleted\"}");
+
+deleteResponse = sendDeleteRequest(StringUtil.join("/", 
this._controllerBaseApiUrl,
+"tables", "table2?type=offline"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables: 
[table2_OFFLINE] deleted\"}");
+
+// Case 5: Delete a non-existent table and expect a bad request expection.
+try {
+  deleteResponse = sendDeleteRequest(
+  StringUtil.join("/", this._controllerBaseApiUrl, "tables", 
"no_such_table_OFFLINE"));
 
 Review comment:
   Add `Assert.fail()` right after this line.


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:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org



[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-09-12 Thread GitBox
jackjlli commented on a change in pull request #4608: Unit tests and bug fixes 
for DeleteTable rest API for controller.
URL: https://github.com/apache/incubator-pinot/pull/4608#discussion_r323993728
 
 

 ##
 File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
 ##
 @@ -254,14 +254,20 @@ public SuccessResponse deleteTable(
 
 List tablesDeleted = new LinkedList<>();
 try {
-  if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+  if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)
+  && _pinotHelixResourceManager.hasOfflineTable(tableName)) {
 _pinotHelixResourceManager.deleteOfflineTable(tableName);
 
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
   }
-  if (tableType != TableType.OFFLINE && 
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+  if (tableType != TableType.OFFLINE && 
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)
+  && _pinotHelixResourceManager.hasRealtimeTable(tableName)) {
 _pinotHelixResourceManager.deleteRealtimeTable(tableName);
 
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
   }
+  if (tablesDeleted.size() == 0) {
 
 Review comment:
   Could you elaborate why you think it's a bug when the table doesn't exist?
   
   Plus, here's an interesting discussion on this topic:
   
https://stackoverflow.com/questions/6439416/deleting-a-resource-using-http-delete


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:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org



[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-09-12 Thread GitBox
jackjlli commented on a change in pull request #4608: Unit tests and bug fixes 
for DeleteTable rest API for controller.
URL: https://github.com/apache/incubator-pinot/pull/4608#discussion_r323993808
 
 

 ##
 File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
 ##
 @@ -254,14 +254,20 @@ public SuccessResponse deleteTable(
 
 List tablesDeleted = new LinkedList<>();
 try {
-  if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+  if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)
+  && _pinotHelixResourceManager.hasOfflineTable(tableName)) {
 _pinotHelixResourceManager.deleteOfflineTable(tableName);
 
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
   }
-  if (tableType != TableType.OFFLINE && 
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+  if (tableType != TableType.OFFLINE && 
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)
+  && _pinotHelixResourceManager.hasRealtimeTable(tableName)) {
 _pinotHelixResourceManager.deleteRealtimeTable(tableName);
 
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
   }
+  if (tablesDeleted.size() == 0) {
+throw new ControllerApplicationException(LOGGER, "Table '" + tableName 
+ "' does not exist",
+Response.Status.BAD_REQUEST);
 
 Review comment:
   I'd prefer `404` instead of a `400`.


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:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org