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

2019-10-07 Thread GitBox
chenboat 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_r332295609
 
 

 ##
 File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
 ##
 @@ -254,20 +254,49 @@ public SuccessResponse deleteTable(
 
 List tablesDeleted = new LinkedList<>();
 try {
-  if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+  boolean tableExist = false;
+  if (verifyTableType(tableName, tableType, tableType.OFFLINE)) {
+tableExist = _pinotHelixResourceManager.hasOfflineTable(tableName);
+// Even the table name does not exist, still go on to delete remaining 
table metadata in case a previous delete
+// did not complete.
 _pinotHelixResourceManager.deleteOfflineTable(tableName);
-
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
+if (tableExist) {
+  
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
+}
   }
-  if (tableType != TableType.OFFLINE && 
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+  if (verifyTableType(tableName, tableType, tableType.REALTIME)) {
+tableExist = _pinotHelixResourceManager.hasRealtimeTable(tableName);
+// Even the table name does not exist, still go on to delete remaining 
table metadata in case a previous delete
+// did not complete.
 _pinotHelixResourceManager.deleteRealtimeTable(tableName);
-
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
+if (tableExist) {
+  
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
+}
+  }
+  if (!tableExist) {
+throw new ControllerApplicationException(LOGGER,
 
 Review comment:
   Good point. Done. @Jackie-Jiang 


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] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-10-07 Thread GitBox
chenboat 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_r332288358
 
 

 ##
 File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
 ##
 @@ -254,20 +254,49 @@ public SuccessResponse deleteTable(
 
 List tablesDeleted = new LinkedList<>();
 try {
-  if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+  boolean tableExist = false;
+  if (verifyTableType(tableName, tableType, tableType.OFFLINE)) {
+tableExist = _pinotHelixResourceManager.hasOfflineTable(tableName);
+// Even the table name does not exist, still go on to delete remaining 
table metadata in case a previous delete
+// did not complete.
 _pinotHelixResourceManager.deleteOfflineTable(tableName);
-
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
+if (tableExist) {
+  
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
+}
   }
-  if (tableType != TableType.OFFLINE && 
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+  if (verifyTableType(tableName, tableType, tableType.REALTIME)) {
+tableExist = _pinotHelixResourceManager.hasRealtimeTable(tableName);
+// Even the table name does not exist, still go on to delete remaining 
table metadata in case a previous delete
+// did not complete.
 _pinotHelixResourceManager.deleteRealtimeTable(tableName);
-
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
+if (tableExist) {
+  
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
+}
+  }
+  if (!tableExist) {
+throw new ControllerApplicationException(LOGGER,
+"Table '" + tableName + "' with type " + tableType + " does not 
exist", Response.Status.NOT_FOUND);
   }
   return new SuccessResponse("Tables: " + tablesDeleted + " deleted");
 } catch (Exception e) {
   throw new ControllerApplicationException(LOGGER, e.getMessage(), 
Response.Status.INTERNAL_SERVER_ERROR, e);
 }
   }
 
+  // Return true iff the table is of the expectedType based on the given 
tableName and tableType. The truth table:
+  //  tableType   
TableNameBuilder.getTableTypeFromTableName(tableName)  Return value
+  //   1.   nullnull (i.e., table has no type 
suffix) true
+  //   2.   nullnot_null   
typeFromTableName == expectedType
+  //   3.  not_null null   
   tableType == expectedType
+  //   4.  not_null not_null  
tableType==typeFromTableName==expectedType
+  private boolean verifyTableType(String tableName, TableType tableType, 
TableType expectedType) {
 
 Review comment:
   Use tab instead of space now. There is no template for comments as far as I 
know.


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] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-10-01 Thread GitBox
chenboat 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_r330387326
 
 

 ##
 File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
 ##
 @@ -254,20 +254,40 @@ public SuccessResponse deleteTable(
 
 List tablesDeleted = new LinkedList<>();
 try {
-  if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+  boolean tableExist = false;
+  if (verifyTableType(tableName, tableType, tableType.OFFLINE)) {
+tableExist = _pinotHelixResourceManager.hasOfflineTable(tableName);
+// Even the table name does not exist, still go on to delete remaining 
table metadata in case a previous delete
+// did not complete.
 _pinotHelixResourceManager.deleteOfflineTable(tableName);
 
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
 
 Review comment:
   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:
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] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-10-01 Thread GitBox
chenboat 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_r330386935
 
 

 ##
 File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
 ##
 @@ -254,20 +254,40 @@ public SuccessResponse deleteTable(
 
 List tablesDeleted = new LinkedList<>();
 try {
-  if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+  boolean tableExist = false;
+  if (verifyTableType(tableName, tableType, tableType.OFFLINE)) {
+tableExist = _pinotHelixResourceManager.hasOfflineTable(tableName);
+// Even the table name does not exist, still go on to delete remaining 
table metadata in case a previous delete
+// did not complete.
 _pinotHelixResourceManager.deleteOfflineTable(tableName);
 
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
   }
-  if (tableType != TableType.OFFLINE && 
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+  if (verifyTableType(tableName, tableType, tableType.REALTIME)) {
+tableExist = _pinotHelixResourceManager.hasRealtimeTable(tableName);
+// Even the table name does not exist, still go on to delete remaining 
table metadata in case a previous delete
+// did not complete.
 _pinotHelixResourceManager.deleteRealtimeTable(tableName);
 
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
   }
+  if (!tableExist) {
+throw new ControllerApplicationException(LOGGER, "Table '" + tableName 
+ "' does not exist",
+Response.Status.NOT_FOUND);
+  }
   return new SuccessResponse("Tables: " + tablesDeleted + " deleted");
 } catch (Exception e) {
   throw new ControllerApplicationException(LOGGER, e.getMessage(), 
Response.Status.INTERNAL_SERVER_ERROR, e);
 }
   }
 
+  // Return true iff the table is of the expectedType based on the given 
tableName and tableType.
+  private boolean verifyTableType(String tableName, TableType tableType, 
TableType expectedType) {
 
 Review comment:
   Good point. Your logic is the same as the current one. Added a truth table 
in the comments to define the boolean logic -- this is probably the best way to 
understand the logic behind.


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] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-10-01 Thread GitBox
chenboat 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_r330386541
 
 

 ##
 File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
 ##
 @@ -254,20 +254,40 @@ public SuccessResponse deleteTable(
 
 List tablesDeleted = new LinkedList<>();
 try {
-  if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+  boolean tableExist = false;
+  if (verifyTableType(tableName, tableType, tableType.OFFLINE)) {
+tableExist = _pinotHelixResourceManager.hasOfflineTable(tableName);
+// Even the table name does not exist, still go on to delete remaining 
table metadata in case a previous delete
+// did not complete.
 _pinotHelixResourceManager.deleteOfflineTable(tableName);
 
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
   }
-  if (tableType != TableType.OFFLINE && 
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+  if (verifyTableType(tableName, tableType, tableType.REALTIME)) {
+tableExist = _pinotHelixResourceManager.hasRealtimeTable(tableName);
+// Even the table name does not exist, still go on to delete remaining 
table metadata in case a previous delete
+// did not complete.
 _pinotHelixResourceManager.deleteRealtimeTable(tableName);
 
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
   }
+  if (!tableExist) {
+throw new ControllerApplicationException(LOGGER, "Table '" + tableName 
+ "' does not exist",
 
 Review comment:
   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:
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] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-09-17 Thread GitBox
chenboat 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_r325495667
 
 

 ##
 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:
   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:
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] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-09-17 Thread GitBox
chenboat 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_r325495647
 
 

 ##
 File path: 
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java
 ##
 @@ -21,10 +21,12 @@
 import com.fasterxml.jackson.databind.JsonNode;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+
 
 Review comment:
   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:
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] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-09-17 Thread GitBox
chenboat 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_r325495546
 
 

 ##
 File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
 ##
 @@ -254,20 +254,41 @@ public SuccessResponse deleteTable(
 
 List tablesDeleted = new LinkedList<>();
 try {
-  if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+  if (toDeleteOfflineTable(tableName, tableType) && 
_pinotHelixResourceManager
+  .hasOfflineTable(tableName)) {
 _pinotHelixResourceManager.deleteOfflineTable(tableName);
 
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
   }
-  if (tableType != TableType.OFFLINE && 
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+  if (toDeleteRealtimeTable(tableName, tableType) && 
_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.NOT_FOUND);
+  }
   return new SuccessResponse("Tables: " + tablesDeleted + " deleted");
 } catch (Exception e) {
-  throw new ControllerApplicationException(LOGGER, e.getMessage(), 
Response.Status.INTERNAL_SERVER_ERROR, e);
+  throw new ControllerApplicationException(LOGGER, e.getMessage(),
+  Response.Status.INTERNAL_SERVER_ERROR, e);
 }
   }
 
+  // Return true if tableType is NOT offline (i.e., null or realtime) and 
table name does  not end
+  // with _offline suffix.
+  private boolean toDeleteRealtimeTable(String tableName, TableType tableType) 
{
 
 Review comment:
   Done. Combine two functions into one: I also use positive condition testing 
to make it easier to understand.


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] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-09-13 Thread GitBox
chenboat 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_r324223469
 
 

 ##
 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:
   Done. Good point.


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] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.

2019-09-13 Thread GitBox
chenboat 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_r324223312
 
 

 ##
 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:
   Right now if the table does not exist, the rest end point still returns 
"Success [table_non_existent] deleted". This is not consistent with system 
state -- that is why I think it is a bug (though minor).
   
   Thank you for the overflow. It is interesting and new to me. Both sides has 
valid points. I think there is argument for idem-potency there -- but Pinot 
controller is not a stateless service (it has zk data for table state) IMO.  


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