[GitHub] [hbase] Apache9 commented on a change in pull request #1818: HBASE-24456 : Create ImmutableScan and use it for CustomizedScanInfoBuilder

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

Apache9 commented on a change in pull request #1818:
URL: https://github.com/apache/hbase/pull/1818#discussion_r433111346



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ImmutableScan.java
##########
@@ -0,0 +1,477 @@
+/*
+ *
+ * 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.client;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.NavigableSet;
+import org.apache.hadoop.hbase.filter.Filter;
+import org.apache.hadoop.hbase.io.TimeRange;
+import org.apache.hadoop.hbase.security.access.Permission;
+import org.apache.hadoop.hbase.security.visibility.Authorizations;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Immutable version of Scan
+ */
+@InterfaceAudience.Public

Review comment:
       Do we really want this to be public?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ImmutableScan.java
##########
@@ -0,0 +1,477 @@
+/*
+ *
+ * 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.client;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.NavigableSet;
+import org.apache.hadoop.hbase.filter.Filter;
+import org.apache.hadoop.hbase.io.TimeRange;
+import org.apache.hadoop.hbase.security.access.Permission;
+import org.apache.hadoop.hbase.security.visibility.Authorizations;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Immutable version of Scan
+ */
+@InterfaceAudience.Public
+public final class ImmutableScan extends Scan {
+
+  /**
+   * Create Immutable instance of Scan from given Scan object
+   *
+   * @param scan Copy all values from Scan
+   * @throws IOException From parent constructor
+   */
+  public ImmutableScan(Scan scan) throws IOException {
+    super(scan);
+    super.setIsolationLevel(scan.getIsolationLevel());
+    Map<byte[], NavigableSet<byte[]>> familyMap = scan.getFamilyMap();
+    for (Map.Entry<byte[], NavigableSet<byte[]>> entry : familyMap.entrySet()) {
+      byte[] family = entry.getKey();
+      NavigableSet<byte[]> cols = entry.getValue();
+      if (cols != null && cols.size() > 0) {
+        for (byte[] col : cols) {
+          super.addColumn(family, col);
+        }
+      } else {
+        super.addFamily(family);
+      }
+    }
+    for (Map.Entry<String, byte[]> attr : scan.getAttributesMap().entrySet()) {
+      super.setAttribute(attr.getKey(), attr.getValue());
+    }
+    for (Map.Entry<byte[], TimeRange> entry : scan.getColumnFamilyTimeRange().entrySet()) {
+      TimeRange tr = entry.getValue();
+      super.setColumnFamilyTimeRange(entry.getKey(), tr.getMin(), tr.getMax());
+    }
+    super.setPriority(scan.getPriority());
+  }
+
+  /**
+   * Create Immutable instance of Scan from given Get object
+   *
+   * @param get Get to model Scan after
+   */
+  public ImmutableScan(Get get) {
+    super(get);
+    super.setIsolationLevel(get.getIsolationLevel());
+    for (Map.Entry<String, byte[]> attr : get.getAttributesMap().entrySet()) {
+      super.setAttribute(attr.getKey(), attr.getValue());
+    }
+    for (Map.Entry<byte[], TimeRange> entry : get.getColumnFamilyTimeRange().entrySet()) {
+      TimeRange tr = entry.getValue();
+      super.setColumnFamilyTimeRange(entry.getKey(), tr.getMin(), tr.getMax());
+    }
+    super.setPriority(get.getPriority());
+  }
+
+  /**
+   * Create a new Scan with a cursor. It only set the position information like start row key.
+   * The others (like cfs, stop row, limit) should still be filled in by the user.
+   * {@link Result#isCursor()}
+   * {@link Result#getCursor()}
+   * {@link Cursor}
+   */
+  public static Scan createScanFromCursor(Cursor cursor) {
+    Scan scan = new Scan().withStartRow(cursor.getRow());
+    try {
+      return new ImmutableScan(scan);
+    } catch (IOException e) {
+      throw new RuntimeException("Scan should not throw IOException", e);
+    }
+  }
+
+  @Override
+  public Scan addFamily(byte[] family) {
+    throw new IllegalStateException("ImmutableScan does not allow access to addFamily");

Review comment:
       UnsupportedOperationException or IllegalStateException?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Query.java
##########
@@ -164,7 +164,11 @@ public int getReplicaId() {
    * @param level IsolationLevel for this query
    */
   public Query setIsolationLevel(IsolationLevel level) {
-    setAttribute(ISOLATION_LEVEL, level.toBytes());
+    if (this instanceof ImmutableScan) {

Review comment:
       Why?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ImmutableScan.java
##########
@@ -0,0 +1,477 @@
+/*
+ *
+ * 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.client;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.NavigableSet;
+import org.apache.hadoop.hbase.filter.Filter;
+import org.apache.hadoop.hbase.io.TimeRange;
+import org.apache.hadoop.hbase.security.access.Permission;
+import org.apache.hadoop.hbase.security.visibility.Authorizations;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Immutable version of Scan
+ */
+@InterfaceAudience.Public
+public final class ImmutableScan extends Scan {
+
+  /**
+   * Create Immutable instance of Scan from given Scan object
+   *
+   * @param scan Copy all values from Scan
+   * @throws IOException From parent constructor
+   */
+  public ImmutableScan(Scan scan) throws IOException {
+    super(scan);
+    super.setIsolationLevel(scan.getIsolationLevel());
+    Map<byte[], NavigableSet<byte[]>> familyMap = scan.getFamilyMap();
+    for (Map.Entry<byte[], NavigableSet<byte[]>> entry : familyMap.entrySet()) {
+      byte[] family = entry.getKey();
+      NavigableSet<byte[]> cols = entry.getValue();
+      if (cols != null && cols.size() > 0) {
+        for (byte[] col : cols) {
+          super.addColumn(family, col);
+        }
+      } else {
+        super.addFamily(family);
+      }
+    }
+    for (Map.Entry<String, byte[]> attr : scan.getAttributesMap().entrySet()) {
+      super.setAttribute(attr.getKey(), attr.getValue());
+    }
+    for (Map.Entry<byte[], TimeRange> entry : scan.getColumnFamilyTimeRange().entrySet()) {
+      TimeRange tr = entry.getValue();
+      super.setColumnFamilyTimeRange(entry.getKey(), tr.getMin(), tr.getMax());
+    }
+    super.setPriority(scan.getPriority());
+  }
+
+  /**
+   * Create Immutable instance of Scan from given Get object
+   *
+   * @param get Get to model Scan after
+   */
+  public ImmutableScan(Get get) {
+    super(get);
+    super.setIsolationLevel(get.getIsolationLevel());
+    for (Map.Entry<String, byte[]> attr : get.getAttributesMap().entrySet()) {
+      super.setAttribute(attr.getKey(), attr.getValue());
+    }
+    for (Map.Entry<byte[], TimeRange> entry : get.getColumnFamilyTimeRange().entrySet()) {
+      TimeRange tr = entry.getValue();
+      super.setColumnFamilyTimeRange(entry.getKey(), tr.getMin(), tr.getMax());
+    }
+    super.setPriority(get.getPriority());
+  }
+
+  /**
+   * Create a new Scan with a cursor. It only set the position information like start row key.
+   * The others (like cfs, stop row, limit) should still be filled in by the user.
+   * {@link Result#isCursor()}
+   * {@link Result#getCursor()}
+   * {@link Cursor}
+   */
+  public static Scan createScanFromCursor(Cursor cursor) {
+    Scan scan = new Scan().withStartRow(cursor.getRow());
+    try {
+      return new ImmutableScan(scan);
+    } catch (IOException e) {
+      throw new RuntimeException("Scan should not throw IOException", e);
+    }
+  }
+
+  @Override
+  public Scan addFamily(byte[] family) {
+    throw new IllegalStateException("ImmutableScan does not allow access to addFamily");
+  }
+
+  @Override
+  public Scan addColumn(byte[] family, byte[] qualifier) {
+    throw new IllegalStateException("ImmutableScan does not allow access to addColumn");
+  }
+
+  @Override
+  public Scan setTimeRange(long minStamp, long maxStamp) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setTimeRange");
+  }
+
+  @Deprecated
+  @Override
+  public Scan setTimeStamp(long timestamp) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setTimeStamp");
+  }
+
+  @Override
+  public Scan setTimestamp(long timestamp) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setTimestamp");
+  }
+
+  @Override
+  public Scan setColumnFamilyTimeRange(byte[] cf, long minStamp, long maxStamp) {
+    throw new IllegalStateException(
+      "ImmutableScan does not allow access to setColumnFamilyTimeRange");
+  }
+
+  @Override
+  public Scan withStartRow(byte[] startRow) {
+    throw new IllegalStateException("ImmutableScan does not allow access to withStartRow");
+  }
+
+  @Override
+  public Scan withStartRow(byte[] startRow, boolean inclusive) {
+    throw new IllegalStateException("ImmutableScan does not allow access to withStartRow");
+  }
+
+  @Override
+  public Scan withStopRow(byte[] stopRow) {
+    throw new IllegalStateException("ImmutableScan does not allow access to withStopRow");
+  }
+
+  @Override
+  public Scan withStopRow(byte[] stopRow, boolean inclusive) {
+    throw new IllegalStateException("ImmutableScan does not allow access to withStopRow");
+  }
+
+  @Override
+  public Scan setRowPrefixFilter(byte[] rowPrefix) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setRowPrefixFilter");
+  }
+
+  @Override
+  public Scan readAllVersions() {
+    throw new IllegalStateException("ImmutableScan does not allow access to readAllVersions");
+  }
+
+  @Override
+  public Scan readVersions(int versions) {
+    throw new IllegalStateException("ImmutableScan does not allow access to readVersions");
+  }
+
+  @Override
+  public Scan setBatch(int batch) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setBatch");
+  }
+
+  @Override
+  public Scan setMaxResultsPerColumnFamily(int limit) {
+    throw new IllegalStateException(
+      "ImmutableScan does not allow access to setMaxResultsPerColumnFamily");
+  }
+
+  @Override
+  public Scan setRowOffsetPerColumnFamily(int offset) {
+    throw new IllegalStateException(
+      "ImmutableScan does not allow access to setRowOffsetPerColumnFamily");
+  }
+
+  @Override
+  public Scan setCaching(int caching) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setCaching");
+  }
+
+  @Override
+  public Scan setMaxResultSize(long maxResultSize) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setMaxResultSize");
+  }
+
+  @Override
+  public Scan setFilter(Filter filter) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setFilter");
+  }
+
+  @Override
+  public Scan setFamilyMap(Map<byte[], NavigableSet<byte[]>> familyMap) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setFamilyMap");
+  }
+
+  @Override
+  public Scan setCacheBlocks(boolean cacheBlocks) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setCacheBlocks");
+  }
+
+  @Override
+  public Scan setReversed(boolean reversed) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setReversed");
+  }
+
+  @Override
+  public Scan setAllowPartialResults(final boolean allowPartialResults) {
+    throw new IllegalStateException(
+      "ImmutableScan does not allow access to setAllowPartialResults");
+  }
+
+  @Override
+  public Scan setLoadColumnFamiliesOnDemand(boolean value) {
+    throw new IllegalStateException(
+      "ImmutableScan does not allow access to setLoadColumnFamiliesOnDemand");
+  }
+
+  @Override
+  public Scan setRaw(boolean raw) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setRaw");
+  }
+
+  @Override
+  @Deprecated
+  public Scan setSmall(boolean small) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setSmall");
+  }
+
+  @Override
+  public Scan setAttribute(String name, byte[] value) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setAttribute");
+  }
+
+  @Override
+  public Scan setId(String id) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setId");
+  }
+
+  @Override
+  public Scan setAuthorizations(Authorizations authorizations) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setAuthorizations");
+  }
+
+  @Override
+  public Scan setACL(Map<String, Permission> perms) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setACL");
+  }
+
+  @Override
+  public Scan setACL(String user, Permission perms) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setACL");
+  }
+
+  @Override
+  public Scan setConsistency(Consistency consistency) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setConsistency");
+  }
+
+  @Override
+  public Scan setReplicaId(int Id) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setReplicaId");
+  }
+
+  @Override
+  public Scan setIsolationLevel(IsolationLevel level) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setIsolationLevel");
+  }
+
+  @Override
+  public Scan setPriority(int priority) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setPriority");
+  }
+
+  @Override
+  public Scan setScanMetricsEnabled(final boolean enabled) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setScanMetricsEnabled");
+  }
+
+  @Override
+  @Deprecated
+  public Scan setAsyncPrefetch(boolean asyncPrefetch) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setAsyncPrefetch");
+  }
+
+  @Override
+  public Scan setLimit(int limit) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setLimit");
+  }
+
+  @Override
+  public Scan setOneRowLimit() {
+    throw new IllegalStateException("ImmutableScan does not allow access to setOneRowLimit");
+  }
+
+  @Override
+  public Scan setReadType(ReadType readType) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setReadType");
+  }
+
+  @Override
+  Scan setMvccReadPoint(long mvccReadPoint) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setMvccReadPoint");
+  }
+
+  @Override
+  Scan resetMvccReadPoint() {
+    throw new IllegalStateException("ImmutableScan does not allow access to resetMvccReadPoint");
+  }
+
+  @Override
+  public Scan setNeedCursorResult(boolean needCursorResult) {
+    throw new IllegalStateException("ImmutableScan does not allow access to setNeedCursorResult");
+  }
+
+  @Override
+  public long getMaxResultSize() {
+    return super.getMaxResultSize();

Review comment:
       Do not need to implement these methods?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Scan.java
##########
@@ -200,35 +200,37 @@ public Scan(Scan scan) throws IOException {
     filter = scan.getFilter(); // clone?
     loadColumnFamiliesOnDemand = scan.getLoadColumnFamiliesOnDemandValue();
     consistency = scan.getConsistency();
-    this.setIsolationLevel(scan.getIsolationLevel());
     reversed = scan.isReversed();
     asyncPrefetch = scan.isAsyncPrefetch();
     small = scan.isSmall();
     allowPartialResults = scan.getAllowPartialResults();
     tr = scan.getTimeRange(); // TimeRange is immutable
-    Map<byte[], NavigableSet<byte[]>> fams = scan.getFamilyMap();
-    for (Map.Entry<byte[],NavigableSet<byte[]>> entry : fams.entrySet()) {
-      byte [] fam = entry.getKey();
-      NavigableSet<byte[]> cols = entry.getValue();
-      if (cols != null && cols.size() > 0) {
-        for (byte[] col : cols) {
-          addColumn(fam, col);
+    if (!(this instanceof ImmutableScan)) {

Review comment:
       This is not a good practise... Cna we avoid this instanceof for testing a sub class in the parent class?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ImmutableScan.java
##########
@@ -0,0 +1,477 @@
+/*
+ *
+ * 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.client;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.NavigableSet;
+import org.apache.hadoop.hbase.filter.Filter;
+import org.apache.hadoop.hbase.io.TimeRange;
+import org.apache.hadoop.hbase.security.access.Permission;
+import org.apache.hadoop.hbase.security.visibility.Authorizations;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Immutable version of Scan
+ */
+@InterfaceAudience.Public
+public final class ImmutableScan extends Scan {
+
+  /**
+   * Create Immutable instance of Scan from given Scan object
+   *
+   * @param scan Copy all values from Scan
+   * @throws IOException From parent constructor
+   */
+  public ImmutableScan(Scan scan) throws IOException {
+    super(scan);
+    super.setIsolationLevel(scan.getIsolationLevel());
+    Map<byte[], NavigableSet<byte[]>> familyMap = scan.getFamilyMap();
+    for (Map.Entry<byte[], NavigableSet<byte[]>> entry : familyMap.entrySet()) {
+      byte[] family = entry.getKey();
+      NavigableSet<byte[]> cols = entry.getValue();
+      if (cols != null && cols.size() > 0) {
+        for (byte[] col : cols) {
+          super.addColumn(family, col);
+        }
+      } else {
+        super.addFamily(family);
+      }
+    }
+    for (Map.Entry<String, byte[]> attr : scan.getAttributesMap().entrySet()) {
+      super.setAttribute(attr.getKey(), attr.getValue());
+    }
+    for (Map.Entry<byte[], TimeRange> entry : scan.getColumnFamilyTimeRange().entrySet()) {
+      TimeRange tr = entry.getValue();
+      super.setColumnFamilyTimeRange(entry.getKey(), tr.getMin(), tr.getMax());
+    }
+    super.setPriority(scan.getPriority());
+  }
+
+  /**
+   * Create Immutable instance of Scan from given Get object
+   *
+   * @param get Get to model Scan after
+   */
+  public ImmutableScan(Get get) {
+    super(get);
+    super.setIsolationLevel(get.getIsolationLevel());
+    for (Map.Entry<String, byte[]> attr : get.getAttributesMap().entrySet()) {
+      super.setAttribute(attr.getKey(), attr.getValue());
+    }
+    for (Map.Entry<byte[], TimeRange> entry : get.getColumnFamilyTimeRange().entrySet()) {
+      TimeRange tr = entry.getValue();
+      super.setColumnFamilyTimeRange(entry.getKey(), tr.getMin(), tr.getMax());
+    }
+    super.setPriority(get.getPriority());
+  }
+
+  /**
+   * Create a new Scan with a cursor. It only set the position information like start row key.
+   * The others (like cfs, stop row, limit) should still be filled in by the user.
+   * {@link Result#isCursor()}
+   * {@link Result#getCursor()}
+   * {@link Cursor}
+   */
+  public static Scan createScanFromCursor(Cursor cursor) {

Review comment:
       Is it necessary to return a ImmutableScan for this method?




----------------------------------------------------------------
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.

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