You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Stephen Jiang <sy...@gmail.com> on 2015/03/16 18:34:20 UTC
Question on EnableTableHandler code
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
Re: Question on EnableTableHandler code
Posted by Stephen Jiang <sy...@gmail.com>.
Now (1) is under control (HBASE-13254). Let us talk about (2). Looks like
we are doing best effort to online all regions of a table during 'enable
table' operation. My argument is that we should be consistent with all
conditions. Currently, we fail if bulk assignment failed with some reason;
but if we don't even do assignment, we declare successful. It is not
consistent; if we are doing best effort, then we should always succeed on
'making regions online' operation (with warning messages - by the way,
warning message is good for debugging, but client could not see it).
Here is the current logic
* done = false;*
* if (we can find servers to do bulk assignment) {*
* if (bulk assignment is complete) {*
* done = true; *
* } else { // either bulk assignment failed or interrupted *
* done = false;*
* }*
* }*
* } else { // bulk plan could not be found*
* done = true;*
* } *
On Mon, Mar 16, 2015 at 11:48 AM, Andrey Stepachev <oc...@gmail.com> 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 <sy...@gmail.com>
> 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 <oc...@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 <oc...@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 <
>>> syuanjiangdev@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.
>>>
>>
>>
>
>
> --
> Andrey.
>
Re: Question on EnableTableHandler code
Posted by Andrey Stepachev <oc...@gmail.com>.
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 <sy...@gmail.com>
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 <
> chrajeshbabu32@gmail.com> 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 <oc...@gmail.com>
>> 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 <syuanjiangdev@gmail.com
>> >
>> > 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 <oc...@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 <oc...@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 <
>> > syuanjiangdev@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.
>> > >>
>> > >
>> > >
>> >
>> >
>> > --
>> > Andrey.
>> >
>>
>
>
--
Andrey.
Re: Question on EnableTableHandler code
Posted by Stephen Jiang <sy...@gmail.com>.
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 <
chrajeshbabu32@gmail.com> 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 <oc...@gmail.com>
> 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 <sy...@gmail.com>
> > 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 <oc...@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 <oc...@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 <
> > syuanjiangdev@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.
> > >>
> > >
> > >
> >
> >
> > --
> > Andrey.
> >
>
Re: Question on EnableTableHandler code
Posted by Rajeshbabu Chintaguntla <ch...@gmail.com>.
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 <oc...@gmail.com> 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 <sy...@gmail.com>
> 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 <oc...@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 <oc...@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 <
> syuanjiangdev@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.
> >>
> >
> >
>
>
> --
> Andrey.
>
Re: Question on EnableTableHandler code
Posted by Andrey Stepachev <oc...@gmail.com>.
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 <sy...@gmail.com>
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 <oc...@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 <oc...@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 <syuanjiangdev@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.
>>
>
>
--
Andrey.
Re: Question on EnableTableHandler code
Posted by Stephen Jiang <sy...@gmail.com>.
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 <oc...@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 <oc...@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 <sy...@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.
>
Re: Question on EnableTableHandler code
Posted by Andrey Stepachev <oc...@gmail.com>.
Stephen, would you like to create jira for case (1)?
Thank you.
On Mon, Mar 16, 2015 at 5:58 PM, Andrey Stepachev <oc...@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 <sy...@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.
Re: Question on EnableTableHandler code
Posted by Andrey Stepachev <oc...@gmail.com>.
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 <sy...@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.