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 <syuanjiang...@gmail.com>
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.
