ndimiduk commented on code in PR #6258: URL: https://github.com/apache/hbase/pull/6258#discussion_r1764586099
########## hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/DiagnosticToolsCommonUtils.java: ########## @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.util; + +import java.io.IOException; +import java.net.InetAddress; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@InterfaceAudience.Private +public class DiagnosticToolsCommonUtils { Review Comment: rename to "KerberosUtils" ? ########## hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/LoadTestUtil.java: ########## @@ -0,0 +1,196 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.util; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Locale; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.MasterNotRunningException; +import org.apache.hadoop.hbase.TableExistsException; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Admin; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; +import org.apache.hadoop.hbase.client.Connection; +import org.apache.hadoop.hbase.client.ConnectionFactory; +import org.apache.hadoop.hbase.client.Durability; +import org.apache.hadoop.hbase.client.TableDescriptor; +import org.apache.hadoop.hbase.client.TableDescriptorBuilder; +import org.apache.hadoop.hbase.io.compress.Compression.Algorithm; +import org.apache.hadoop.hbase.io.encoding.DataBlockEncoding; +import org.apache.hadoop.hbase.util.RegionSplitter.SplitAlgorithm; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@InterfaceAudience.Private +public class LoadTestUtil { + public static final String OPT_DATA_BLOCK_ENCODING_USAGE = "Encoding algorithm (e.g. prefix " + + "compression) to use for data blocks in the test column family, " + "one of " + + Arrays.toString(DataBlockEncoding.values()) + "."; + public static final String OPT_DATA_BLOCK_ENCODING = + ColumnFamilyDescriptorBuilder.DATA_BLOCK_ENCODING.toLowerCase(Locale.ROOT); + /** Column family used by the test */ + public static byte[] DEFAULT_COLUMN_FAMILY = Bytes.toBytes("test_cf"); Review Comment: Details of the schema used by a particular utility should be isolated to that utility, right? Or is there some intention behind sharing schema across these utilities? I'd rather see these kinds of constants repeated in each utility where they're used. Or maybe this change is big enough as it is, we can save further cleanup for a separate PR. ########## hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/KeyProviderForTesting.java: ########## @@ -19,10 +19,12 @@ import java.security.Key; import javax.crypto.spec.SecretKeySpec; +import org.apache.yetus.audience.InterfaceAudience; /** * Return a fixed secret key for AES for testing. */ +@InterfaceAudience.Private public class KeyProviderForTesting implements KeyProvider { Review Comment: This is not longer "for testing", right? ########## hbase-it/pom.xml: ########## @@ -106,6 +106,17 @@ </exclusion> </exclusions> </dependency> + <dependency> Review Comment: +1 Looks like integration tests are using the same schema constants and table creation helpers. See my above comment about being careful about explicitly shared schema vs. coincidentally shared schema. ########## hbase-assembly/src/main/assembly/hadoop-three-compat.xml: ########## @@ -46,6 +46,7 @@ <include>org.apache.hbase:hbase-it</include> <include>org.apache.hbase:hbase-logging</include> <include>org.apache.hbase:hbase-mapreduce</include> + <include>org.apache.hbase:hbase-diagnostics</include> Review Comment: this is a good name. the other obvious choice that comes to mind is "hbase-performance-evaluation". ########## hbase-asyncfs/src/test/java/org/apache/hadoop/hbase/security/HBaseKerberosUtils.java: ########## @@ -172,23 +170,6 @@ public static void setSSLConfiguration(HBaseCommonTestingUtil utility, Class<?> KeyStoreTestUtil.setupSSLConfig(keystoresDir.getAbsolutePath(), sslConfDir, conf, false); } - public static UserGroupInformation loginAndReturnUGI(Configuration conf, String username) Review Comment: or a dedicated hbase-kerberos module. Now that we have TLS and it can be used with or without kerberos, i think it makes sense for these to be distinct modules (see [HBASE-28757](https://issues.apache.org/jira/browse/HBASE-28757)). ########## hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat2.java: ########## @@ -554,18 +554,43 @@ private byte[][] generateRandomStartKeys(int numKeys) { // first region start key is always empty ret[0] = HConstants.EMPTY_BYTE_ARRAY; for (int i = 1; i < numKeys; i++) { - ret[i] = - PerformanceEvaluation.generateData(random, PerformanceEvaluation.DEFAULT_VALUE_LENGTH); + ret[i] = generateData(random, DEFAULT_VALUE_LENGTH); } return ret; } + /* Review Comment: I like having the data used in a unit test local to the unit test. There's not much gained by sharing this kind of code around different utilities -- we're not avoiding bug in production via DRY, we're just being lazy in our test harness. I can see there being some utility class per test module or per test module+package that contains code shared across tests in that module/package. Sharing project-wide isn't tat helpful, and it frees up tests to be locally, independently adjusted as needed. ########## hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/master/balancer/LoadBalancerPerformanceEvaluation.java: ########## @@ -177,7 +175,7 @@ protected int doWork() throws Exception { public static void main(String[] args) throws IOException { LoadBalancerPerformanceEvaluation tool = new LoadBalancerPerformanceEvaluation(); - tool.setConf(UTIL.getConfiguration()); + tool.setConf(HBaseConfiguration.create()); Review Comment: Now it's going to load up the local system's configurations. Maybe it already did that previously... ########## hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/LoadTestUtil.java: ########## @@ -0,0 +1,196 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.util; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Locale; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.MasterNotRunningException; +import org.apache.hadoop.hbase.TableExistsException; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Admin; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; +import org.apache.hadoop.hbase.client.Connection; +import org.apache.hadoop.hbase.client.ConnectionFactory; +import org.apache.hadoop.hbase.client.Durability; +import org.apache.hadoop.hbase.client.TableDescriptor; +import org.apache.hadoop.hbase.client.TableDescriptorBuilder; +import org.apache.hadoop.hbase.io.compress.Compression.Algorithm; +import org.apache.hadoop.hbase.io.encoding.DataBlockEncoding; +import org.apache.hadoop.hbase.util.RegionSplitter.SplitAlgorithm; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@InterfaceAudience.Private +public class LoadTestUtil { Review Comment: Can we break that outside dependency, keep this isolated to hbase-diagnostics? I think it's better to keep hbase-common slim. -- 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: issues-unsubscr...@hbase.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
