hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Phabricator (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-5335) Dynamic Schema Configurations
Date Mon, 19 Mar 2012 22:01:48 GMT

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

Phabricator commented on HBASE-5335:
------------------------------------

mbautin has commented on the revision "[jira] [HBASE-5335] Dynamic Schema Config".

  Nicolas: Looks good! Some more comments inline.

  Also, we now have lint enabled. Could you please re-run "mvn -Darc initialize", then do
"arc lint" or "arc diff --preview" and address lint comments? Thanks!


INLINE COMMENTS
  src/main/java/org/apache/hadoop/hbase/HConstants.java:361 This constant needs a better name.
Probably some of the constants above it do, too.
  src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:530 Is it possible to reduce
code duplication between here and HColumnDescriptor?
  src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:549 single quote escaping
  src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:579-581 single quote escaping
  src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:367 Looks similar to the corresponding
function in HColumnDescriptor. Is it possible to reuse code between the two? (getValues()
would be a bigger case for that.)
  src/main/java/org/apache/hadoop/hbase/util/CompoundConfiguration.java:42 Add <p/>
  src/main/java/org/apache/hadoop/hbase/util/CompoundConfiguration.java:222-229 Why exactly
do we have to copy-and-paste the code instead of using composition or inheritance?

  If there is a bug in Configuration and it is fixed there, it will not be reflected here,
which is indeed somewhat "tragic".
  src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java:4246 It would be great
to avoid adding more tests to TestFromClientSide and create a separate test class instead.
  src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java:4247 This probably
should not use javadoc syntax.
  src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java:4312 Is it possible
to wait for an event instead of a specific amount of time?
  src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:753 In that case we should
probably sanitize user input for single quotes somewhere else, wherever the user is supposed
to tweak configuration values in real time. However, I think it is easier to escape single
quotes here. It would also be good to parse the output value of this function with jruby in
a test.
  src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java:4322 Create an HConstant
for this key
  src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java:4292 Sleep times should
be according to http://hbase.apache.org/book.html#hbase.tests.writing . We should probably
have a constant for each type of sleep time. Quoting the linked page from HBase book:

  > Whenever possible, tests should not use Thread.sleep, but rather waiting for the real
event they need. This is faster and clearer for the reader. Tests should not do a Thread.sleep
without testing an ending condition. This allows understanding what the test is waiting for.
Moreover, the test will work whatever the machine performance is. Sleep should be minimal
to be as fast as possible. Waiting for a variable should be done in a 40ms sleep loop. Waiting
for a socket operation should be done in a 200 ms sleep loop.
  src/test/java/org/apache/hadoop/hbase/util/TestCompoundConfiguration.java:33 Can this be
private?
  src/test/java/org/apache/hadoop/hbase/util/TestCompoundConfiguration.java:43-46 Is this
override necessary?
  src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:760 I guess we need a javadoc
saying that ADVANCED is not for general-purpose use, and that it is meant for use as an HColumnDescriptor
key, then.
  src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:783 OK, if you think we won't
be calling this a lot, this is fine with me.

REVISION DETAIL
  https://reviews.facebook.net/D2247

                
> Dynamic Schema Configurations
> -----------------------------
>
>                 Key: HBASE-5335
>                 URL: https://issues.apache.org/jira/browse/HBASE-5335
>             Project: HBase
>          Issue Type: New Feature
>            Reporter: Nicolas Spiegelberg
>            Assignee: Nicolas Spiegelberg
>              Labels: configuration, schema
>         Attachments: D2247.1.patch, D2247.2.patch, D2247.3.patch
>
>
> Currently, the ability for a core developer to add per-table & per-CF configuration
settings is very heavyweight.  You need to add a reserved keyword all the way up the stack
& you have to support this variable long-term if you're going to expose it explicitly
to the user.  This has ended up with using Configuration.get() a lot because it is lightweight
and you can tweak settings while you're trying to understand system behavior [since there
are many config params that may never need to be tuned].  We need to add the ability to put
& read arbitrary KV settings in the HBase schema.  Combined with online schema change,
this will allow us to safely iterate on configuration settings.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message