[
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
