[GitHub] [hbase] Apache9 commented on a change in pull request #1655: HBASE-24321 - Add writable MinVersions and read-only Scan to coproc S…

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

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



##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
##########
@@ -685,6 +687,40 @@ public StoreFileReader postStoreFileReaderOpen(ObserverContext<RegionCoprocessor
     return reader;
   }
 
+  @Override
+  public void preStoreScannerOpen(ObserverContext<RegionCoprocessorEnvironment> ctx,
+    Store store, ScanOptions options) throws IOException {
+    if (options.getScan().getTimeRange().isAllTime()) {

Review comment:
       I think this class is only used to test whether a specific CP method has been called? What is this used for?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanInfo.java
##########
@@ -174,8 +174,13 @@ public boolean isNewVersionBehavior() {
    * Used for CP users for customizing max versions, ttl and keepDeletedCells.
    */
   ScanInfo customize(int maxVersions, long ttl, KeepDeletedCells keepDeletedCells) {

Review comment:
       Is this method still referenced? Just remove it?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CustomizedScanInfoBuilder.java
##########
@@ -34,8 +36,18 @@
 
   private KeepDeletedCells keepDeletedCells = null;
 
+  private Integer minVersions;
+
+  private final Scan scan;
+
   public CustomizedScanInfoBuilder(ScanInfo scanInfo) {
     this.scanInfo = scanInfo;
+    this.scan = new Scan();
+  }
+  public CustomizedScanInfoBuilder(ScanInfo scanInfo, Scan scan) throws IOException {

Review comment:
       Let's just catch the IOException in the code here and then throw an AssertionError to indicate this should not happen, and we can filed another issue to remove the IOException from throws.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanOptions.java
##########
@@ -64,4 +66,10 @@ default void readAllVersions() {
   void setKeepDeletedCells(KeepDeletedCells keepDeletedCells);
 
   KeepDeletedCells getKeepDeletedCells();
+
+  int getMinVersions();
+
+  void setMinVersions(int minVersions);
+
+  Scan getScan() throws IOException;

Review comment:
       Please add a comment to say that the returned Scan object is only for reading, changing it will have no effect to the actual scan operation.
   And please modify the javadoc of this class to indicate that now we can also change min versions?




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