[
https://issues.apache.org/jira/browse/HBASE-4213?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13104280#comment-13104280
]
[email protected] commented on HBASE-4213:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1786/#review1885
-----------------------------------------------------------
/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
<https://reviews.apache.org/r/1786/#comment4337>
coming late to the game on the review here...
but why do we need a separate "instantAdd" boolean. If this feature works,
we should always use it. Otherwise we have to maintain multiple code paths, and
the API is more complicated. Plus, this breaks a public API.
/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<https://reviews.apache.org/r/1786/#comment4338>
the fact that these two constructors differ only by number of arguments is
quite suspect. Same with the two functions below.
/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
<https://reviews.apache.org/r/1786/#comment4339>
we need to somehow expose this to the user in a way aside from just the
logs. For example, we could make a MonitoredTask for the table operation, and
mark it as failed here.
Ideally it would actually be a response to the original RPC that the user
called in HTableAdmin.
/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
<https://reviews.apache.org/r/1786/#comment4340>
we need some kind of try/finally here. Plus, this can easily race against
other things enabling/disabling the balancer. We need a kind of "balancer
disable stack" so that if multiple operations need to disable the balancer for
a period, they can all work together. Plus, if the balancer is mid-balance,
does balanceSwitch(false) wait for the balance to finish?
/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
<https://reviews.apache.org/r/1786/#comment4341>
definitely need a sleep or yield. Seems like a code smell as well
/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
<https://reviews.apache.org/r/1786/#comment4342>
why not just make this the while condition?
Since ZK is notification-based, can't we wait on this occuring rather than
polling in a loop?
/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/1786/#comment4343>
!isEmpty() instead of size() comparison
/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/1786/#comment4344>
what happens if on the reopen, an exception is thrown? does the region get
orphaned? we need some kind of recovery path here.
Example case: what if you alter a table and change compression to a codec
that this RS doesn't have?
/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/1786/#comment4345>
seems like DEBUG level.
This whole process should be TaskMonitored.
/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/1786/#comment4351>
this function never returns null. So at call sites, don't check for null -
just adds cruft.
/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/1786/#comment4346>
thread safety properties here? I'm surprised we don't need any locking. For
example, what happens to regions that are in the process of opening?
/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/1786/#comment4348>
!isEmpty()
/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/1786/#comment4349>
DEBUG level
/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/1786/#comment4350>
DEBUG
/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/1786/#comment4347>
should be ERROR or at least WARN level.
Need to include the exception traceback.
/src/main/java/org/apache/hadoop/hbase/regionserver/OnlineRegions.java
<https://reviews.apache.org/r/1786/#comment4352>
empty javadoc, plus it doesn't @return anything, it's void
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4353>
this should be registered _before_ the above line, right?
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4354>
if this fails, shouldn't we abort the server or something? We don't want a
half-functional RS running that doesn't respond to schema changes.
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4355>
empty javadoc
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4356>
debug
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4358>
debug
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4359>
why is this the right behavior? could use a comment here, because it seems
wrong.
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4360>
why create and then set data, instead of just creating with the correct
data?
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4357>
rename to "schemaChangeNodeExists" or "doesSchemaChangeNodeExist"
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4361>
dead code
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4362>
would servers ever be null?
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4363>
what would cause this and what are the ramifications?
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4364>
this would be on *any* ZK change anywhere, right? Maybe better to invert
this whole condition and just "return;" if it's for a path you're not
interested in, reducing the nesting level of the function
/src/main/java/org/apache/hadoop/hbase/zookeeper/SchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4365>
this order seems wrong, again
/src/main/java/org/apache/hadoop/hbase/zookeeper/SchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4366>
debug
/src/main/java/org/apache/hadoop/hbase/zookeeper/SchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4367>
again error paths seem suspect
/src/main/java/org/apache/hadoop/hbase/zookeeper/SchemaChangeTracker.java
<https://reviews.apache.org/r/1786/#comment4368>
again: when would these error paths get hit, and what state would we be
left in? needs comments describing why it's safe to ignore the errors
/src/main/ruby/hbase/admin.rb
<https://reviews.apache.org/r/1786/#comment4369>
similar to above, I see no reason to duplicate APIs here... having multiple
alter commands is just going to confuse users and mean we have more code paths
to test
/src/test/java/org/apache/hadoop/hbase/client/TestInstantSchemaChange.java
<https://reviews.apache.org/r/1786/#comment4371>
need test cases covering failure cases. EG modify a column to change the
codec to a random string, or some other modification that will cause an error
on open.
another test case: what if you use the "instant" API while the table is
disabled?
what about while the table is in the process of opening?
/src/test/java/org/apache/hadoop/hbase/client/TestInstantSchemaChange.java
<https://reviews.apache.org/r/1786/#comment4370>
these while conditions should at least yield if not sleep. Plus why not
just put the condition in the while, rather than if/break?
- Todd
On 2011-09-12 18:36:02, Ted Yu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/1786/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-09-12 18:36:02)
bq.
bq.
bq. Review request for hbase.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. From Subbu:
bq. here is the latest patch that support alter_instant, an instant schema
change command that supports (Add, Modify, Delete column and Modify table)
actions through ZK.
bq.
bq. 1. This pattern capitalizes on the fact that HRI's are now in HDFS and
need not be sent over again from Master to RS cloud on every schema change
event.
bq.
bq. 2. Offers real time instant schema change as we bypass the explicit bulk
reassign (unassign + assign) of regions from master to RS.
bq.
bq. 3. Offers fault tolerant schema change support as schema changes now go
through ZK. Secondary master taking over a failed schema change will be
addressed through a separate JIRA.
bq.
bq.
bq. This addresses bug HBASE-4213.
bq. https://issues.apache.org/jira/browse/HBASE-4213
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. /src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java 1169522
bq. /src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 1169522
bq. /src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
1169522
bq. /src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 1169522
bq. /src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1169522
bq. /src/main/java/org/apache/hadoop/hbase/master/MasterServices.java
1169522
bq.
/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java
1169522
bq.
/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java
1169522
bq.
/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java
1169522
bq.
/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java
1169522
bq.
/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
1169522
bq.
/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java
1169522
bq. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
1169522
bq. /src/main/java/org/apache/hadoop/hbase/regionserver/OnlineRegions.java
1169522
bq. /src/main/java/org/apache/hadoop/hbase/rest/SchemaResource.java 1169522
bq.
/src/main/java/org/apache/hadoop/hbase/zookeeper/MasterSchemaChangeTracker.java
PRE-CREATION
bq.
/src/main/java/org/apache/hadoop/hbase/zookeeper/SchemaChangeTracker.java
PRE-CREATION
bq. /src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
1169522
bq. /src/main/ruby/hbase/admin.rb 1169522
bq. /src/main/ruby/shell.rb 1169522
bq. /src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1169522
bq. /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
1169522
bq.
/src/test/java/org/apache/hadoop/hbase/client/TestInstantSchemaChange.java
PRE-CREATION
bq.
/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java
1169522
bq. /src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java
1169522
bq.
/src/test/java/org/apache/hadoop/hbase/regionserver/handler/MockRegionServerServices.java
1169522
bq.
bq. Diff: https://reviews.apache.org/r/1786/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Unit tests pass.
bq.
bq.
bq. Thanks,
bq.
bq. Ted
bq.
bq.
> Support for fault tolerant, instant schema updates with out master's
> intervention (i.e with out enable/disable and bulk assign/unassign) through
> ZK.
> ----------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HBASE-4213
> URL: https://issues.apache.org/jira/browse/HBASE-4213
> Project: HBase
> Issue Type: Improvement
> Reporter: Subbu M Iyer
> Assignee: Subbu M Iyer
> Fix For: 0.92.0
>
> Attachments: 4213-Instant_Schema_change_through_ZK.patch,
> 4213-V5-Support_instant_schema_changes_through_ZK.patch,
> 4213-V7-Support_instant_schema_changes_through_ZK.patch,
> 4213-V8-Support_instant_schema_changes_through_ZK.patch, 4213.v6,
> HBASE-4213-Instant_schema_change.patch,
> HBASE-4213_Instant_schema_change_-Version_2_.patch,
> HBASE_Instant_schema_change-version_3_.patch
>
>
> This Jira is a slight variation in approach to what is being done as part of
> https://issues.apache.org/jira/browse/HBASE-1730
> Support instant schema updates such as Modify Table, Add Column, Modify
> Column operations:
> 1. With out enable/disabling the table.
> 2. With out bulk unassign/assign of regions.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
