[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.
jamesyfshao 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_r325444901 ## 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: I think it would be better to think of this change as an improvement to the pinot admin API feedback. set aside the debates over the correct way to respond to a DELETE on a resource that is not presented, This change provides the caller of this method more information by providing feedback if such method doesn't exist. If the users desire the old behavior, he/she can just check if the method returns non-500 return codes. But now our users are also able to check if the table is really in pinot system. Given how this change improves the existing pinot system behavior, I think we should accept this diff after fixing any further minor issues @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] jamesyfshao commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.
jamesyfshao 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_r325443739 ## 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: I feel this two method can use better naming, for example, ensureTableNameMatchType(..) and potentially merge two method into one. We can probably even remove the comments if the method naming is improved 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