Stephen , you are right , that is my code and that thing was overlooked :) I think we need completely remove state cleanup code. Actually how it done tablestate manager could not return table which later will be rendered as nonexistent. Basically that means, that if we got nonexistent table in EnableTable handler we can fail even in recovery, because it means that something really bad happened and seems we can ask to run hbck.
So in short, we just need to throw TableNotFoundException and remove flag skipTableStateCheck from EnableTableHandler. On Mon, Mar 16, 2015 at 7:49 PM, Stephen Jiang <[email protected]> wrote: > thanks, Rajeshbabu, HBASE-10215 is not the last change, The HBASE-7767 > (hello, Andrey [?]) removed the exception throw code after setting up the > table state, what we really want is as follows (if Andrey agrees with the > change, I will create a JIRA and send out the patch today): > > // 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) { > > this.assignmentManager.getTableStateManager().setDeletedTable( > tableName); > > } > > throw new TableNotFoundException(tableName); > > } > > > > On Mon, Mar 16, 2015 at 12:09 PM, Rajeshbabu Chintaguntla < > [email protected]> wrote: > >> Hi Stephen and Andrey, >> >> The first step added to remove stale znodes if table creation fails after >> znode creation. >> See HBASE-10215 <https://issues.apache.org/jira/browse/HBASE-10215>. Not >> sure still we need it or not. >> >> Thanks, >> Rajeshbabu. >> >> >> >> >> On Tue, Mar 17, 2015 at 12:18 AM, Andrey Stepachev <[email protected]> >> wrote: >> >> > Thanks Stephen. >> > >> > on (2): I think that much better to guarantee that table was enabled >> (i.e. >> > all internal structures reflect that fact and balancer knows about new >> > table). But result of that could be checked asyncronically from Admin. >> > Does it make sense? >> > >> > On Mon, Mar 16, 2015 at 6:10 PM, Stephen Jiang <[email protected] >> > >> > wrote: >> > >> > > Andrey, I will take care of (1). >> > > >> > > And (2) :-) if your guys agree. Because it is not consistent, if the >> > bulk >> > > assigned failed, we would fail the enabling table; however, if the >> bulk >> > > assign not starts, we would enable table with offline regions - really >> > > inconsistent - we either all fail in those scenarios or all succeed >> with >> > > offline region (best effort approach). >> > > >> > > Thanks >> > > Stephen >> > > >> > > On Mon, Mar 16, 2015 at 11:01 AM, Andrey Stepachev <[email protected]> >> > > wrote: >> > > >> > >> 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. >> > >> >> > > >> > > >> > >> > >> > -- >> > Andrey. >> > >> > > -- Andrey.
