[GitHub] [hbase] Apache9 commented on a diff in pull request #4645: HBASE-27206 Clean up error-prone findings in hbase-common

[GitBox <gi...@apache.org>]

Apache9 commented on code in PR #4645:
URL: https://github.com/apache/hbase/pull/4645#discussion_r928120158


##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:
##########
@@ -279,14 +278,14 @@ private Cell toOffheapCell(ByteBuffer valAndTagsBuffer, int vOffset,
   protected static class OnheapDecodedCell implements ExtendedCell {
     private static final long FIXED_OVERHEAD = ClassSize.align(ClassSize.OBJECT
       + (3 * ClassSize.REFERENCE) + (2 * Bytes.SIZEOF_LONG) + (7 * Bytes.SIZEOF_INT)
-      + (Bytes.SIZEOF_SHORT) + (2 * Bytes.SIZEOF_BYTE) + (3 * ClassSize.ARRAY));
+      + Bytes.SIZEOF_SHORT + (2 * Bytes.SIZEOF_BYTE) + (3 * ClassSize.ARRAY));
     private byte[] keyOnlyBuffer;
     private short rowLength;
     private int familyOffset;
     private byte familyLength;
     private int qualifierOffset;
     private int qualifierLength;
-    private long timestamp;
+    private long timeStamp;

Review Comment:
   I think timestamp is a valid word in English?



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/TagCompressionContext.java:
##########
@@ -81,7 +81,8 @@ public void compressTags(OutputStream out, byte[] in, int offset, int length) th
   public void compressTags(OutputStream out, ByteBuffer in, int offset, int length)
     throws IOException {
     if (in.hasArray()) {
-      compressTags(out, in.array(), offset, length);
+      // Offset we are given is relative to ByteBuffer#arrayOffset
+      compressTags(out, in.array(), in.arrayOffset() + offset, length);

Review Comment:
   So this should be a bug?



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/VersionInfo.java:
##########
@@ -162,7 +166,7 @@ private static String[] getVersionComponents(final String version) {
   }
 
   public static int getMajorVersion(String version) {
-    return Integer.parseInt(version.split("\\.")[0]);
+    return Integer.parseInt(Iterables.get(Splitter.on('.').split(version), 0));

Review Comment:
   Ah, could use Iterables.get instead of Iterators.get to save a iterator call in our own code base. Good.



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/WindowMovingAverage.java:
##########
@@ -47,15 +47,15 @@ public WindowMovingAverage(String label, int size) {
 
   @Override
   protected void updateMostRecentTime(long elapsed) {
-    int index = moveForwardMostRecentPosistion();
+    int index = moveForwardMostRecentPosition();

Review Comment:
   Thanks for fixing the typo!



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java:
##########
@@ -100,6 +101,8 @@ public void writeIdInBytes(OutputStream stream) throws IOException {
    * @param dest   output array
    * @param offset starting offset of the output array n
    */
+  // System.arraycopy is static native. Nothing we can do this until we have minimum JDK 9.

Review Comment:
   Thanks for the comment.



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:
##########
@@ -271,7 +272,7 @@ public byte[] encodeData() {
         if (this.meta.isIncludesMvcc()) {
           memstoreTS = ByteBufferUtils.readVLong(in);
         }
-        kv = new KeyValue(in.array(), kvOffset,
+        kv = new KeyValue(in.array(), in.arrayOffset() + kvOffset,

Review Comment:
   Bug?



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileContextBuilder.java:
##########
@@ -44,11 +44,11 @@ public class HFileContextBuilder {
   /** Whether tags to be compressed or not **/
   private boolean compressTags = false;
   /** the checksum type **/
-  private ChecksumType checksumType = ChecksumType.getDefaultChecksumType();
+  private ChecksumType checkSumType = ChecksumType.getDefaultChecksumType();

Review Comment:
   I think checksum is also a valid word in English? The class name is ChecksumType, not CheckSumType...



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/DNS.java:
##########
@@ -65,7 +66,7 @@ public enum ServerType {
     MASTER("master"),
     REGIONSERVER("regionserver");
 
-    private String name;
+    private final String name;
 
     ServerType(String name) {

Review Comment:
   Also make the constuctor private?



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/Pair.java:
##########
@@ -102,9 +100,11 @@ public boolean equals(Object other) {
 
   @Override
   public int hashCode() {
-    if (first == null) return (second == null) ? 0 : second.hashCode() + 1;
-    else if (second == null) return first.hashCode() + 2;
-    else return first.hashCode() * 17 + second.hashCode();
+    if (first == null) {
+      return (second == null) ? 0 : second.hashCode() + 1;
+    } else if (second == null) {
+      return first.hashCode() + 2;
+    } else return first.hashCode() * 17 + second.hashCode();

Review Comment:
   Also wrap the last statement with {}?



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/ServerName.java:
##########
@@ -212,21 +215,31 @@ public int getPort() {
     return this.address.getPort();
   }
 
+  /**
+   * Return the start code.
+   * @deprecated Use {@link #getStartCode()} instead.

Review Comment:
   Better to mention the deprecation cycle. Like 'Since 2.5.0, will be removed in 4.0.0'.



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/AbstractByteRange.java:
##########
@@ -87,6 +87,7 @@ public abstract class AbstractByteRange implements ByteRange {
   //
   // methods for managing the backing array and range viewport
   //
+

Review Comment:
   Why this change? And better change the above comments to javadoc?



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/ServerName.java:
##########
@@ -142,7 +144,9 @@ public static ServerName valueOf(final String hostname, final int port, final lo
    */
   public static ServerName valueOf(final String serverName) {
     final String hostname = serverName.substring(0, serverName.indexOf(SERVERNAME_SEPARATOR));
-    final int port = Integer.parseInt(serverName.split(SERVERNAME_SEPARATOR)[1]);
+    Iterator<String> i = Splitter.onPattern(SERVERNAME_SEPARATOR).split(serverName).iterator();

Review Comment:
   I prefer we just implement this method like this
   ```
       int firstSep = serverName.indexOf(SERVERNAME_SEPARATOR);
       int lastSep = serverName.lastIndexOf(SERVERNAME_SEPARATOR);
       String hostname = serverName.substring(0, firstSep);
       int port = Integer.parseInt(serverName.substring(firstSep + 1, lastSep));
       long startCode = Long.parseLong(serverName.substring(lastSep + 1));
       return valueOf(hostname, port, startCode);
   ```



-- 
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-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org