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 <oct...@gmail.com> wrote:
> Stephen, would you like to create jira for case (1)?
>
> Thank you.
>
> On Mon, Mar 16, 2015 at 5:58 PM, Andrey Stepachev <oct...@gmail.com>
> 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 <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.
> >
>
>
>
> --
> Andrey.
>
