Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public int compare(final Cell l, final Cell r, boolean ignoreSequenceid) {
return ignoreSequenceid ? diff : Long.compare(r.getSequenceId(), l.getSequenceId());
}

private static int compareKeyValues(final KeyValue left, final KeyValue right) {
private int compareKeyValues(final KeyValue left, final KeyValue right) {
int diff;
// Compare Rows. Cache row length.
int leftRowLength = left.getRowLength();
Expand Down Expand Up @@ -153,8 +153,8 @@ private static int compareKeyValues(final KeyValue left, final KeyValue right) {
// Compare families.
int leftFamilyPosition = left.getFamilyOffset(leftFamilyLengthPosition);
int rightFamilyPosition = right.getFamilyOffset(rightFamilyLengthPosition);
diff = Bytes.compareTo(left.getFamilyArray(), leftFamilyPosition, leftFamilyLength,
right.getFamilyArray(), rightFamilyPosition, rightFamilyLength);
diff = compareFamilies(left, leftFamilyPosition, leftFamilyLength, right, rightFamilyPosition,
rightFamilyLength);
if (diff != 0) {
return diff;
}
Expand Down Expand Up @@ -183,7 +183,7 @@ private static int compareKeyValues(final KeyValue left, final KeyValue right) {
return (0xff & rightType) - (0xff & leftType);
}

private static int compareBBKV(final ByteBufferKeyValue left, final ByteBufferKeyValue right) {
private int compareBBKV(final ByteBufferKeyValue left, final ByteBufferKeyValue right) {
int diff;
// Compare Rows. Cache row length.
int leftRowLength = left.getRowLength();
Expand Down Expand Up @@ -236,8 +236,8 @@ private static int compareBBKV(final ByteBufferKeyValue left, final ByteBufferKe
// Compare families.
int leftFamilyPosition = left.getFamilyPosition(leftFamilyLengthPosition);
int rightFamilyPosition = right.getFamilyPosition(rightFamilyLengthPosition);
diff = ByteBufferUtils.compareTo(left.getFamilyByteBuffer(), leftFamilyPosition,
leftFamilyLength, right.getFamilyByteBuffer(), rightFamilyPosition, rightFamilyLength);
diff = compareFamilies(left, leftFamilyPosition, leftFamilyLength, right, rightFamilyPosition,
rightFamilyLength);
if (diff != 0) {
return diff;
}
Expand Down Expand Up @@ -265,7 +265,7 @@ private static int compareBBKV(final ByteBufferKeyValue left, final ByteBufferKe
return (0xff & rightType) - (0xff & leftType);
}

private static int compareKVVsBBKV(final KeyValue left, final ByteBufferKeyValue right) {
private int compareKVVsBBKV(final KeyValue left, final ByteBufferKeyValue right) {
int diff;
// Compare Rows. Cache row length.
int leftRowLength = left.getRowLength();
Expand Down Expand Up @@ -318,8 +318,8 @@ private static int compareKVVsBBKV(final KeyValue left, final ByteBufferKeyValue
// Compare families.
int leftFamilyPosition = left.getFamilyOffset(leftFamilyLengthPosition);
int rightFamilyPosition = right.getFamilyPosition(rightFamilyLengthPosition);
diff = ByteBufferUtils.compareTo(left.getFamilyArray(), leftFamilyPosition, leftFamilyLength,
right.getFamilyByteBuffer(), rightFamilyPosition, rightFamilyLength);
diff = compareFamilies(left, leftFamilyPosition, leftFamilyLength, right, rightFamilyPosition,
rightFamilyLength);
if (diff != 0) {
return diff;
}
Expand Down Expand Up @@ -368,7 +368,10 @@ private int compareColumns(final Cell left, final int leftFamLen, final int left
return compareQualifiers(left, leftQualLen, right, rightQualLen);
}

private int compareFamilies(Cell left, int leftFamLen, Cell right, int rightFamLen) {
/**
* This method will be overridden when we compare cells inner store to bypass family comparing.
*/
protected int compareFamilies(Cell left, int leftFamLen, Cell right, int rightFamLen) {
if (left instanceof ByteBufferExtendedCell && right instanceof ByteBufferExtendedCell) {
return ByteBufferUtils.compareTo(((ByteBufferExtendedCell) left).getFamilyByteBuffer(),
((ByteBufferExtendedCell) left).getFamilyPosition(), leftFamLen,
Expand Down Expand Up @@ -448,6 +451,34 @@ public final int compareFamilies(Cell left, Cell right) {
right.getFamilyArray(), right.getFamilyOffset(), right.getFamilyLength());
}

/**
* This method will be overridden when we compare cells inner store to bypass family comparing.
*/
protected int compareFamilies(KeyValue left, int leftFamilyPosition, int leftFamilyLength,
KeyValue right, int rightFamilyPosition, int rightFamilyLength) {
return Bytes.compareTo(left.getFamilyArray(), leftFamilyPosition, leftFamilyLength,
right.getFamilyArray(), rightFamilyPosition, rightFamilyLength);
}

/**
* This method will be overridden when we compare cells inner store to bypass family comparing.
*/
protected int compareFamilies(ByteBufferKeyValue left, int leftFamilyPosition,
int leftFamilyLength, ByteBufferKeyValue right, int rightFamilyPosition,
int rightFamilyLength) {
return ByteBufferUtils.compareTo(left.getFamilyByteBuffer(), leftFamilyPosition,
leftFamilyLength, right.getFamilyByteBuffer(), rightFamilyPosition, rightFamilyLength);
}

/**
* This method will be overridden when we compare cells inner store to bypass family comparing.
*/
protected int compareFamilies(KeyValue left, int leftFamilyPosition, int leftFamilyLength,
ByteBufferKeyValue right, int rightFamilyPosition, int rightFamilyLength) {
return ByteBufferUtils.compareTo(left.getFamilyArray(), leftFamilyPosition, leftFamilyLength,
right.getFamilyByteBuffer(), rightFamilyPosition, rightFamilyLength);
}

static int compareQualifiers(KeyValue left, KeyValue right) {
// NOTE: Same method is in CellComparatorImpl, also private, not shared, intentionally. Not
// sharing gets us a few percent more throughput in compares. If changes here or there, make
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1603,6 +1603,14 @@ public enum OperationStatusCode {
*/
public final static boolean HBASE_SERVER_USEIP_ENABLED_DEFAULT = false;

/**
* Whether to use {@link MetaCellComparator} even if we are not meta region. Used when creating
* master local region.
*/
public static final String USE_META_CELL_COMPARATOR = "hbase.region.use.meta.cell.comparator";

public static final boolean DEFAULT_USE_META_CELL_COMPARATOR = false;

private HConstants() {
// Can't be instantiated with this ctor.
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
* 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;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.yetus.audience.InterfaceAudience;
import org.apache.yetus.audience.InterfaceStability;

/**
* Compare two HBase cells inner store, skip compare family for better performance. Important!!! we
* should not make fake cell with fake family which length greater than zero inner store, otherwise
* this optimization cannot be used.
*/
@InterfaceAudience.Private
@InterfaceStability.Evolving
public class InnerStoreCellComparator extends CellComparatorImpl {

private static final long serialVersionUID = 8186411895799094989L;

public static final InnerStoreCellComparator INNER_STORE_COMPARATOR =
new InnerStoreCellComparator();

@Override
protected int compareFamilies(Cell left, int leftFamilyLength, Cell right,
int rightFamilyLength) {
return leftFamilyLength - rightFamilyLength;
}

@Override
protected int compareFamilies(KeyValue left, int leftFamilyPosition, int leftFamilyLength,
KeyValue right, int rightFamilyPosition, int rightFamilyLength) {
return leftFamilyLength - rightFamilyLength;
}

@Override
protected int compareFamilies(ByteBufferKeyValue left, int leftFamilyPosition,
int leftFamilyLength, ByteBufferKeyValue right, int rightFamilyPosition,
int rightFamilyLength) {
return leftFamilyLength - rightFamilyLength;
}

@Override
protected int compareFamilies(KeyValue left, int leftFamilyPosition, int leftFamilyLength,
ByteBufferKeyValue right, int rightFamilyPosition, int rightFamilyLength) {
return leftFamilyLength - rightFamilyLength;
}

/**
* Utility method that makes a guess at comparator to use based off passed tableName. Use in
* extreme when no comparator specified.
* @return CellComparator to use going off the {@code tableName} passed.
*/
public static CellComparator getInnerStoreCellComparator(Configuration conf,
TableName tableName) {
return getInnerStoreCellComparator(conf, tableName.toBytes());
}

/**
* Utility method that makes a guess at comparator to use based off passed tableName. Use in
* extreme when no comparator specified.
* @return CellComparator to use going off the {@code tableName} passed.
*/
public static CellComparator getInnerStoreCellComparator(byte[] tableName) {
return getInnerStoreCellComparator(null, tableName);
}

public static CellComparator getInnerStoreCellComparator(Configuration conf, byte[] tableName) {
if (
conf != null && conf.getBoolean(HConstants.USE_META_CELL_COMPARATOR,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this judgement in upper layer so we do not need to move USE_META_CELL_COMPARATOR to HConstants? It is just for internal use, we'd better not put it into an IA.Public class...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try.

HConstants.DEFAULT_USE_META_CELL_COMPARATOR)
) {
return MetaCellComparator.META_COMPARATOR;
}
return Bytes.equals(tableName, TableName.META_TABLE_NAME.toBytes())
? MetaCellComparator.META_COMPARATOR
: InnerStoreCellComparator.INNER_STORE_COMPARATOR;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
package org.apache.hadoop.hbase.io.hfile;

import org.apache.hadoop.hbase.CellComparator;
import org.apache.hadoop.hbase.CellComparatorImpl;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.InnerStoreCellComparator;
import org.apache.hadoop.hbase.io.HeapSize;
import org.apache.hadoop.hbase.io.compress.Compression;
import org.apache.hadoop.hbase.io.crypto.Encryption;
Expand Down Expand Up @@ -121,8 +121,9 @@ public HFileContext(HFileContext context) {
// If no cellComparator specified, make a guess based off tablename. If hbase:meta, then should
// be the meta table comparator. Comparators are per table.
this.cellComparator = cellComparator != null ? cellComparator
: this.tableName != null ? CellComparatorImpl.getCellComparator(this.tableName)
: CellComparator.getInstance();
: this.tableName != null
? InnerStoreCellComparator.getInnerStoreCellComparator(this.tableName)
: InnerStoreCellComparator.INNER_STORE_COMPARATOR;
}

/** Returns true when on-disk blocks are compressed, and/or encrypted; false otherwise. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,13 @@ public class TestCellComparator {
HBaseClassTestRule.forClass(TestCellComparator.class);

private CellComparator comparator = CellComparator.getInstance();
private CellComparator innerStoreComparator = InnerStoreCellComparator.INNER_STORE_COMPARATOR;

byte[] row1 = Bytes.toBytes("row1");
byte[] row2 = Bytes.toBytes("row2");
byte[] row_1_0 = Bytes.toBytes("row10");

byte[] fam0 = HConstants.EMPTY_BYTE_ARRAY;
byte[] fam1 = Bytes.toBytes("fam1");
byte[] fam2 = Bytes.toBytes("fam2");
byte[] fam_1_2 = Bytes.toBytes("fam12");
Expand Down Expand Up @@ -76,6 +79,29 @@ public void testCompareCells() {
assertTrue(CellUtil.equals(kv1, kv2));
}

@Test
public void testCompareCellsWithEmptyFamily() {
KeyValue kv1 = new KeyValue(row1, fam0, qual1, val);
KeyValue kv2 = new KeyValue(row1, fam1, qual1, val);
assertTrue(comparator.compare(kv1, kv2) < 0);
assertTrue(innerStoreComparator.compare(kv1, kv2) < 0);

kv1 = new KeyValue(row1, fam0, qual2, val);
kv2 = new KeyValue(row1, fam0, qual1, val);
assertTrue(comparator.compare(kv1, kv2) > 0);
assertTrue(innerStoreComparator.compare(kv1, kv2) > 0);

kv1 = new KeyValue(row1, fam0, qual2, val);
kv2 = new KeyValue(row1, fam0, qual1, val);
assertTrue(comparator.compareFamilies(kv1, kv2) == 0);
assertTrue(innerStoreComparator.compareFamilies(kv1, kv2) == 0);

kv1 = new KeyValue(row1, fam1, qual2, val);
kv2 = new KeyValue(row1, fam1, qual1, val);
assertTrue(comparator.compareFamilies(kv1, kv2) == 0);
assertTrue(innerStoreComparator.compareFamilies(kv1, kv2) == 0);
}

@Test
public void testCompareCellWithKey() throws Exception {
KeyValue kv1 = new KeyValue(row1, fam1, qual1, val);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.apache.hadoop.fs.FSDataInputStream;
import org.apache.hadoop.hbase.CellComparator;
import org.apache.hadoop.hbase.CellComparatorImpl;
import org.apache.hadoop.hbase.InnerStoreCellComparator;
import org.apache.hadoop.hbase.KeyValue;
import org.apache.hadoop.hbase.MetaCellComparator;
import org.apache.hadoop.hbase.io.compress.Compression;
Expand Down Expand Up @@ -121,7 +122,8 @@ public class FixedFileTrailer {
* Raw key comparator class name in version 3
*/
// We could write the actual class name from 2.0 onwards and handle BC
private String comparatorClassName = CellComparator.getInstance().getClass().getName();
private String comparatorClassName =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No compatibility issues?

Copy link
Contributor Author

@bsglz bsglz May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No compatibility issues?

Thought about it and i think so, because we do the conversion when we save and read the hfile, the actual store is the KVComparator, we can't change this since we want hbase1.x to be able to read the files generated in the new version. See the comment of getHBase1CompatibleName for more detail.
Thanks.

InnerStoreCellComparator.INNER_STORE_COMPARATOR.getClass().getName();

/**
* The encryption key
Expand Down Expand Up @@ -574,7 +576,10 @@ public void setComparatorClass(Class<? extends CellComparator> klass) {
*/
@Deprecated
private String getHBase1CompatibleName(final String comparator) {
if (comparator.equals(CellComparatorImpl.class.getName())) {
if (
comparator.equals(CellComparatorImpl.class.getName())
|| comparator.equals(InnerStoreCellComparator.class.getName())
) {
return KeyValue.COMPARATOR.getClass().getName();
}
if (comparator.equals(MetaCellComparator.class.getName())) {
Expand All @@ -593,7 +598,7 @@ private static Class<? extends CellComparator> getComparatorClass(String compara
|| comparatorClassName.equals(KeyValue.COMPARATOR.getClass().getName())
|| (comparatorClassName.equals("org.apache.hadoop.hbase.CellComparator"))
) {
comparatorKlass = CellComparatorImpl.class;
comparatorKlass = InnerStoreCellComparator.class;
} else if (
comparatorClassName.equals(KeyValue.META_COMPARATOR.getLegacyKeyComparatorName())
|| comparatorClassName.equals(KeyValue.META_COMPARATOR.getClass().getName())
Expand Down Expand Up @@ -622,8 +627,11 @@ private static Class<? extends CellComparator> getComparatorClass(String compara
}

static CellComparator createComparator(String comparatorClassName) throws IOException {
if (comparatorClassName.equals(CellComparatorImpl.COMPARATOR.getClass().getName())) {
return CellComparatorImpl.COMPARATOR;
if (
comparatorClassName
.equals(InnerStoreCellComparator.INNER_STORE_COMPARATOR.getClass().getName())
) {
return InnerStoreCellComparator.INNER_STORE_COMPARATOR;
} else
if (comparatorClassName.equals(MetaCellComparator.META_COMPARATOR.getClass().getName())) {
return MetaCellComparator.META_COMPARATOR;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.hbase.HBaseIOException;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.Server;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
Expand Down Expand Up @@ -370,7 +371,7 @@ public static MasterRegion create(MasterRegionParams params) throws IOException
conf.setBoolean(HRegion.WAL_HSYNC_CONF_KEY, params.useHsync());
}
if (params.useMetaCellComparator() != null) {
conf.setBoolean(HRegion.USE_META_CELL_COMPARATOR, params.useMetaCellComparator());
conf.setBoolean(HConstants.USE_META_CELL_COMPARATOR, params.useMetaCellComparator());
}
conf.setInt(AbstractFSWAL.RING_BUFFER_SLOT_COUNT,
IntMath.ceilingPowerOfTwo(params.ringBufferSlotCount()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.apache.hadoop.hbase.CellComparator;
import org.apache.hadoop.hbase.HBaseConfiguration;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.InnerStoreCellComparator;
import org.apache.hadoop.hbase.KeyValue;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.ClassSize;
Expand Down Expand Up @@ -58,7 +59,7 @@ public class DefaultMemStore extends AbstractMemStore {
* Default constructor. Used for tests.
*/
public DefaultMemStore() {
this(HBaseConfiguration.create(), CellComparator.getInstance(), null);
this(HBaseConfiguration.create(), InnerStoreCellComparator.INNER_STORE_COMPARATOR, null);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,14 +277,6 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
public static final String RECOVERED_EDITS_IGNORE_EOF =
"hbase.hregion.recovered.edits.ignore.eof";

/**
* Whether to use {@link MetaCellComparator} even if we are not meta region. Used when creating
* master local region.
*/
public static final String USE_META_CELL_COMPARATOR = "hbase.region.use.meta.cell.comparator";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge deal, but why move this constant in this PR? Doesn't seem related?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge deal, but why move this constant in this PR? Doesn't seem related?

Because the new InnerStoreCellComparator which placed in hbase-common module reference it too, and the hbase-common should not depend on hbase-server.
Thanks.


public static final boolean DEFAULT_USE_META_CELL_COMPARATOR = false;

final AtomicBoolean closed = new AtomicBoolean(false);

/*
Expand Down Expand Up @@ -792,8 +784,8 @@ public HRegion(final HRegionFileSystem fs, final WAL wal, final Configuration co
// 'conf' renamed to 'confParam' b/c we use this.conf in the constructor
this.baseConf = confParam;
this.conf = new CompoundConfiguration().add(confParam).addBytesMap(htd.getValues());
this.cellComparator = htd.isMetaTable()
|| conf.getBoolean(USE_META_CELL_COMPARATOR, DEFAULT_USE_META_CELL_COMPARATOR)
this.cellComparator = htd.isMetaTable() || conf.getBoolean(HConstants.USE_META_CELL_COMPARATOR,
HConstants.DEFAULT_USE_META_CELL_COMPARATOR)
? MetaCellComparator.META_COMPARATOR
: CellComparatorImpl.COMPARATOR;
this.lock = new ReentrantReadWriteLock(
Expand Down
Loading