Stephen, would you like to create jira for case (1)? Thank you.
On Mon, Mar 16, 2015 at 5:58 PM, Andrey Stepachev <[email protected]> wrote: > Thanks Stephen. > > Looks like you are right. For (1) case we really don't need there > state cleanup. That is a bug. Should throw TableNotFoundException. > > As for (2) in case of no online region servers available we could > leave table enabled, but no regions would be assigned. > > Actually that rises good question what enable table means, > i.e. do we really need to guarantee that on table enable absolutely > all regions are online, or that could be done in Admin on client side. > > So for now it seems that Enable handler do what is best, and leave > table enabled but unassigned to be later assigned by Balancer. > > On Mon, Mar 16, 2015 at 5:34 PM, Stephen Jiang <[email protected]> > wrote: > >> I want to make sure that the following logic in EnableTableHandler is >> correct: >> >> (1). In EnableTableHandler#prepare - if the table is not existed, it >> marked >> the table as deleted and not throw exception. The result is the table >> lock >> is released and the caller has no knowledge that the table not exist or >> already deleted, it would continue the next step. >> >> Currently, this would happen during recovery (the caller is >> AssignmentManager#recoverTableInEnablingState()) - however, looking at >> recovery code, it expects TableNotFoundException Should we always throw >> exception - if the table not exist? I want to make sure that I don't >> break >> recovery logic by modifying. >> >> public EnableTableHandler prepare() { >> >> ... >> >> // Check if table exists >> >> if (!MetaTableAccessor.tableExists(this.server.getConnection(), >> tableName)) { >> >> // retainAssignment is true only during recovery. In normal case >> it is false >> >> if (!this.skipTableStateCheck) { >> >> throw new TableNotFoundException(tableName); >> >> } >> >> this.assignmentManager.getTableStateManager().setDeletedTable( >> tableName); >> >> } >> >> ... >> >> } >> (2). In EnableTableHandler#handleEnableTable() - if the bulk assign plan >> could not be find, it would leave regions to be offline and declare enable >> table succeed - i think this is a bug and we should retry or fail - but I >> want to make sure that there are some merit behind this logic >> >> private void handleEnableTable() { >> >> Map<ServerName, List<HRegionInfo>> bulkPlan = >> >> this.assignmentManager.getBalancer().retainAssignment( >> regionsToAssign, onlineServers); >> >> if (bulkPlan != null) { >> >> ... >> >> } else { >> >> LOG.info("Balancer was unable to find suitable servers for table " + >> tableName >> >> + ", leaving unassigned"); >> >> done = true; >> >> } >> >> if (done) { >> >> // Flip the table to enabled. >> >> this.assignmentManager.getTableStateManager().setTableState( >> >> this.tableName, TableState.State.ENABLED); >> >> LOG.info("Table '" + this.tableName >> >> + "' was successfully enabled. Status: done=" + done); >> >> } >> >> ... >> >> } >> >> >> thanks >> Stephen >> > > > > -- > Andrey. > -- Andrey.
