virajjasani commented on a change in pull request #3372:
URL: https://github.com/apache/hbase/pull/3372#discussion_r666286329
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptor.java
##########
@@ -275,9 +275,9 @@
/**
* Check if normalization enable flag of the table is true. If flag is false
- * then no region normalizer won't attempt to normalize this table.
+ * then region normalizer won't attempt to normalize this table.
*
- * @return true if region normalization is enabled for this table
Review comment:
nit: I think this one looks concise and sufficient, good to keep it as
is.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/RegionNormalizer.java
##########
@@ -36,6 +36,7 @@
*/
@InterfaceAudience.Private
interface RegionNormalizer extends Configurable {
+ String NORMALIZATION_ENABLED_CONF_KEY = "hbase.table.normalization.enabled";
Review comment:
Good to define this directly in `RegionNormalizerWorker`
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/RegionNormalizerWorker.java
##########
@@ -71,6 +73,12 @@
this.splitPlanCount = 0;
this.mergePlanCount = 0;
this.rateLimiter = loadRateLimiter(configuration);
+ this.defaultNormalizerValue = extractDefaultNormalizerValue(configuration);
+ }
+
+ private boolean extractDefaultNormalizerValue(final Configuration
configuration) {
+ String s =
configuration.get(RegionNormalizer.NORMALIZATION_ENABLED_CONF_KEY);
+ return Boolean.valueOf(s);
Review comment:
Simplify to `Boolean.parseBoolean()`?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java
##########
@@ -866,11 +860,11 @@ public ModifyableTableDescriptor setMergeEnabled(final
boolean isEnable) {
* Check if normalization enable flag of the table is true. If flag is
false
* then no region normalizer won't attempt to normalize this table.
*
- * @return true if region normalization is enabled for this table
Review comment:
same as above
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/RegionNormalizerWorker.java
##########
@@ -181,10 +189,19 @@ public void run() {
final TableDescriptor tblDesc;
try {
tblDesc = masterServices.getTableDescriptors().get(tableName);
- if (tblDesc != null && !tblDesc.isNormalizationEnabled()) {
- LOG.debug("Skipping table {} because normalization is disabled in its
table properties.",
- tableName);
- return Collections.emptyList();
+ boolean normalizationEnabled;
+ if (tblDesc != null) {
+ String defined =
tblDesc.getValue(TableDescriptorBuilder.NORMALIZATION_ENABLED);
+ if(defined != null) {
+ normalizationEnabled = tblDesc.isNormalizationEnabled();
+ } else {
+ normalizationEnabled = this.defaultNormalizerValue;
+ }
+ if (!normalizationEnabled) {
+ LOG.debug("Skipping table {} because normalization is disabled in
its table properties.",
+ tableName);
Review comment:
Let's make it
```
if (!normalizationEnabled) {
LOG.debug("Skipping table {} because normalization is disabled in
its table properties and normalization is also disabled at table level by
default (config: {}).",
tableName, NORMALIZATION_ENABLED_CONF_KEY);
return Collections.emptyList();
}
```
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/RegionNormalizerWorker.java
##########
@@ -55,6 +56,7 @@
private final RateLimiter rateLimiter;
private final long[] skippedCount;
+ private final boolean defaultNormalizerValue;
Review comment:
Good to rename it to `defaultNormalizerTableLevel` or something similar
to indicate that this value is going to be used if not overridden by
`NORMALIZATION_ENABLED` at table descriptor level. Good to add this as comment
also.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
