hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-4213) Support for fault tolerant, instant schema updates with out master's intervention (i.e with out enable/disable and bulk assign/unassign) through ZK.
Date Wed, 14 Sep 2011 06:12:11 GMT

    [ https://issues.apache.org/jira/browse/HBASE-4213?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13104280#comment-13104280
] 

jiraposter@reviews.apache.org 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

        

Mime
View raw message