[GitHub] [hbase] saintstack commented on a change in pull request #3436: HBASE-26036 DBB released too early in HRegion.get() and dirty data for some operations

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

saintstack commented on a change in pull request #3436:
URL: https://github.com/apache/hbase/pull/3436#discussion_r663271264



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Region.java
##########
@@ -476,6 +476,7 @@ boolean checkAndRowMutate(byte [] row, Filter filter, TimeRange timeRange,
 
   /**
    * Do a get based on the get parameter.
+   * Only used for test purpose.

Review comment:
       What do you mean by this? That 'get' is for test only? Do you want to say more in this javadoc?

##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/ByteBuffAllocator.java
##########
@@ -70,6 +71,8 @@
 
   public static final String MIN_ALLOCATE_SIZE_KEY = "hbase.server.allocator.minimal.allocate.size";
 
+  public static final String BYTEBUFF_ALLOCATOR_CLASS = "hbase.bytebuff.allocator.class";

Review comment:
       One suggestion is to add a class comment that it is possible to put in place an alternate bytebuffallocator by setting this param.
   
   Second, I'd suggest adding the zeroing-out bytebuffer into this package so that it is available for test.
   
   For example, I'd like to enable the zeroing-out bytebuffer while running a large cluster test to see if there are other places where we are prematurely releasing backing bytebuffers.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -3244,18 +3244,21 @@ private void prepareDeleteTimestamps(Mutation mutation, Map<byte[], List<Cell>>
 
   private void updateDeleteLatestVersionTimestamp(Cell cell, Get get, int count, byte[] byteNow)
       throws IOException {
-    List<Cell> result = get(get, false);
+    try (RegionScanner scanner = getScanner(new Scan(get))) {
+      List<Cell> result = new ArrayList<>();

Review comment:
       Same here. I think it needs a comment to prevent a developer 'optimizing' replacing w/ a get.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MultiRowMutationEndpoint.java
##########
@@ -221,22 +223,25 @@ private boolean matches(Region region, ClientProtos.Condition condition) throws
       get.setTimeRange(timeRange.getMin(), timeRange.getMax());
     }
 
-    List<Cell> result = region.get(get, false);
     boolean matches = false;
-    if (filter != null) {
-      if (!result.isEmpty()) {
-        matches = true;
-      }
-    } else {
-      boolean valueIsNull = comparator.getValue() == null || comparator.getValue().length == 0;
-      if (result.isEmpty() && valueIsNull) {
-        matches = true;
-      } else if (result.size() > 0 && result.get(0).getValueLength() == 0 && valueIsNull) {
-        matches = true;
-      } else if (result.size() == 1 && !valueIsNull) {
-        Cell kv = result.get(0);
-        int compareResult = PrivateCellUtil.compareValue(kv, comparator);
-        matches = matches(op, compareResult);
+    try (RegionScanner scanner = region.getScanner(new Scan(get))) {
+      List<Cell> result = new ArrayList<>();

Review comment:
       nit:  a developer seeing this code w/o the proper background might look at this code block and think -- why scan to do one next ? Why not just replace it w/ a 'get'? Does this need a comment? If only to point at the JIRA?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -4044,60 +4047,58 @@ private static Get toGet(final Mutation mutation) throws IOException {
         get.setTimeRange(tr.getMin(), tr.getMax());
       }
 
-      List<Cell> currentValues = region.get(get, false);
-
-      // Iterate the input columns and update existing values if they were found, otherwise
-      // add new column initialized to the delta amount
-      int currentValuesIndex = 0;
-      for (int i = 0; i < deltas.size(); i++) {
-        Cell delta = deltas.get(i);
-        Cell currentValue = null;
-        if (currentValuesIndex < currentValues.size() &&
-          CellUtil.matchingQualifier(currentValues.get(currentValuesIndex), delta)) {
-          currentValue = currentValues.get(currentValuesIndex);
-          if (i < (deltas.size() - 1) && !CellUtil.matchingQualifier(delta, deltas.get(i + 1))) {
-            currentValuesIndex++;
+      List<Cell> currentValues = new ArrayList<>();

Review comment:
       Super nit.... Does currentValues have to be out here? Usually you put it after you open the try {.




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