Apache9 commented on code in PR #5545:
URL: https://github.com/apache/hbase/pull/5545#discussion_r1545362732
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java:
##########
@@ -105,72 +132,44 @@ public class StoreFileWriter implements CellSink,
ShipperListener {
* @param fileContext The HFile context
* @param shouldDropCacheBehind Drop pages written to page cache after
writing the store file.
* @param compactedFilesSupplier Returns the {@link HStore} compacted files
which not archived
+ * @param comparator Cell comparator
+ * @param maxVersions max cell versions
+ * @param newVersionBehavior enable new version behavior
* @throws IOException problem writing to FS
*/
- private StoreFileWriter(FileSystem fs, Path path, final Configuration conf,
CacheConfig cacheConf,
- BloomType bloomType, long maxKeys, InetSocketAddress[] favoredNodes,
HFileContext fileContext,
- boolean shouldDropCacheBehind, Supplier<Collection<HStoreFile>>
compactedFilesSupplier)
- throws IOException {
+ private StoreFileWriter(FileSystem fs, Path liveFilePath, Path
historicalFilePath,
+ final Configuration conf, CacheConfig cacheConf, BloomType bloomType, long
maxKeys,
+ InetSocketAddress[] favoredNodes, HFileContext fileContext, boolean
shouldDropCacheBehind,
+ Supplier<Collection<HStoreFile>> compactedFilesSupplier, CellComparator
comparator,
+ int maxVersions, boolean newVersionBehavior) throws IOException {
+ this.fs = fs;
+ this.historicalFilePath = historicalFilePath;
+ this.conf = conf;
+ this.cacheConf = cacheConf;
+ this.bloomType = bloomType;
+ this.maxKeys = maxKeys;
+ this.favoredNodes = favoredNodes;
+ this.fileContext = fileContext;
+ this.shouldDropCacheBehind = shouldDropCacheBehind;
this.compactedFilesSupplier = compactedFilesSupplier;
- this.timeRangeTracker =
TimeRangeTracker.create(TimeRangeTracker.Type.NON_SYNC);
- // TODO : Change all writers to be specifically created for compaction
context
- writer =
- HFile.getWriterFactory(conf, cacheConf).withPath(fs,
path).withFavoredNodes(favoredNodes)
-
.withFileContext(fileContext).withShouldDropCacheBehind(shouldDropCacheBehind).create();
-
- generalBloomFilterWriter =
BloomFilterFactory.createGeneralBloomAtWrite(conf, cacheConf,
- bloomType, (int) Math.min(maxKeys, Integer.MAX_VALUE), writer);
-
- if (generalBloomFilterWriter != null) {
- this.bloomType = bloomType;
- this.bloomParam = BloomFilterUtil.getBloomFilterParam(bloomType, conf);
- if (LOG.isTraceEnabled()) {
- LOG.trace("Bloom filter type for " + path + ": " + this.bloomType + ",
param: "
- + (bloomType == BloomType.ROWPREFIX_FIXED_LENGTH
- ? Bytes.toInt(bloomParam)
- : Bytes.toStringBinary(bloomParam))
- + ", " + generalBloomFilterWriter.getClass().getSimpleName());
- }
- // init bloom context
- switch (bloomType) {
- case ROW:
- bloomContext =
- new RowBloomContext(generalBloomFilterWriter,
fileContext.getCellComparator());
- break;
- case ROWCOL:
- bloomContext =
- new RowColBloomContext(generalBloomFilterWriter,
fileContext.getCellComparator());
- break;
- case ROWPREFIX_FIXED_LENGTH:
- bloomContext = new
RowPrefixFixedLengthBloomContext(generalBloomFilterWriter,
- fileContext.getCellComparator(), Bytes.toInt(bloomParam));
- break;
- default:
- throw new IOException(
- "Invalid Bloom filter type: " + bloomType + " (ROW or ROWCOL or
ROWPREFIX expected)");
- }
- } else {
- // Not using Bloom filters.
- this.bloomType = BloomType.NONE;
- }
+ this.comparator = comparator;
+ this.maxVersions = maxVersions;
+ this.newVersionBehavior = newVersionBehavior;
+ liveFileWriter = new SingleStoreFileWriter(fs, liveFilePath, conf,
cacheConf, bloomType,
+ maxKeys, favoredNodes, fileContext, shouldDropCacheBehind,
compactedFilesSupplier);
+ }
- // initialize delete family Bloom filter when there is NO RowCol Bloom
filter
- if (this.bloomType != BloomType.ROWCOL) {
- this.deleteFamilyBloomFilterWriter =
BloomFilterFactory.createDeleteBloomAtWrite(conf,
- cacheConf, (int) Math.min(maxKeys, Integer.MAX_VALUE), writer);
- deleteFamilyBloomContext =
- new RowBloomContext(deleteFamilyBloomFilterWriter,
fileContext.getCellComparator());
- } else {
- deleteFamilyBloomFilterWriter = null;
- }
- if (deleteFamilyBloomFilterWriter != null && LOG.isTraceEnabled()) {
- LOG.trace("Delete Family Bloom filter type for " + path + ": "
- + deleteFamilyBloomFilterWriter.getClass().getSimpleName());
- }
+ public static boolean shouldEnableHistoricalCompactionFiles(Configuration
conf) {
+ return conf.getBoolean(ENABLE_HISTORICAL_COMPACTION_FILES,
+ DEFAULT_ENABLE_HISTORICAL_COMPACTION_FILES)
+ && conf.get(STORE_ENGINE_CLASS_KEY, DefaultStoreEngine.class.getName())
+ .equals(DefaultStoreEngine.class.getName())
+ && conf.get(DEFAULT_COMPACTOR_CLASS_KEY,
DefaultCompactor.class.getName())
+ .equals(DefaultCompactor.class.getName());
}
public long getPos() throws IOException {
Review Comment:
Is this the correct behavior after we support writing multiple files? Or do
we still need this method?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStoreFile.java:
##########
@@ -138,6 +140,12 @@ public class HStoreFile implements StoreFile {
// Indicates if the file got compacted
private volatile boolean compactedAway = false;
+ // Indicate if the file contains historical cell versions. This is used when
+ // hbase.enable.historical.compaction.files is set to true. In that case,
compactions
+ // can generate two files, one with the live cell versions and the other
with the remaining
+ // (historical) cell versions.
+ private volatile boolean isHistorical = false;
Review Comment:
I'm still a bit confusd here. What does it mean if we have false here? This
file only contains live cell versions? What if we just disable live file
tracking? The field will be true or false?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java:
##########
@@ -256,156 +234,571 @@ public void appendMobMetadata(SetMultimap<TableName,
String> mobRefSet) throws I
public void appendTrackedTimestampsToMetadata() throws IOException {
// TODO: The StoreFileReader always converts the byte[] to TimeRange
// via TimeRangeTracker, so we should write the serialization data of
TimeRange directly.
- appendFileInfo(TIMERANGE_KEY,
TimeRangeTracker.toByteArray(timeRangeTracker));
- appendFileInfo(EARLIEST_PUT_TS, Bytes.toBytes(earliestPutTs));
+ liveFileWriter.appendTrackedTimestampsToMetadata();
+ if (historicalFileWriter != null) {
+ historicalFileWriter.appendTrackedTimestampsToMetadata();
+ }
}
/**
* Record the earlest Put timestamp. If the timeRangeTracker is not set,
update TimeRangeTracker
* to include the timestamp of this key
*/
public void trackTimestamps(final Cell cell) {
- if (KeyValue.Type.Put.getCode() == cell.getTypeByte()) {
- earliestPutTs = Math.min(earliestPutTs, cell.getTimestamp());
+ liveFileWriter.trackTimestamps(cell);
+ if (historicalFileWriter != null) {
+ historicalFileWriter.trackTimestamps(cell);
}
- timeRangeTracker.includeTimestamp(cell);
}
- private void appendGeneralBloomfilter(final Cell cell) throws IOException {
- if (this.generalBloomFilterWriter != null) {
- /*
- *
http://2.bp.blogspot.com/_Cib_A77V54U/StZMrzaKufI/AAAAAAAAADo/ZhK7bGoJdMQ/s400/KeyValue.png
- * Key = RowLen + Row + FamilyLen + Column [Family + Qualifier] +
Timestamp 3 Types of
- * Filtering: 1. Row = Row 2. RowCol = Row + Qualifier 3.
RowPrefixFixedLength = Fixed Length
- * Row Prefix
- */
- bloomContext.writeBloom(cell);
+ @Override
+ public void beforeShipped() throws IOException {
+ liveFileWriter.beforeShipped();
+ if (historicalFileWriter != null) {
+ historicalFileWriter.beforeShipped();
}
}
- private void appendDeleteFamilyBloomFilter(final Cell cell) throws
IOException {
- if (!PrivateCellUtil.isDeleteFamily(cell) &&
!PrivateCellUtil.isDeleteFamilyVersion(cell)) {
- return;
- }
+ public Path getPath() {
+ return liveFileWriter.getPath();
+ }
- // increase the number of delete family in the store file
- deleteFamilyCnt++;
- if (this.deleteFamilyBloomFilterWriter != null) {
- deleteFamilyBloomContext.writeBloom(cell);
+ public List<Path> getPaths() {
+ if (historicalFileWriter == null) {
+ return Lists.newArrayList(liveFileWriter.getPath());
}
+ return Lists.newArrayList(liveFileWriter.getPath(),
historicalFileWriter.getPath());
}
- @Override
- public void append(final Cell cell) throws IOException {
- appendGeneralBloomfilter(cell);
- appendDeleteFamilyBloomFilter(cell);
- writer.append(cell);
- trackTimestamps(cell);
+ public boolean hasGeneralBloom() {
+ return liveFileWriter.hasGeneralBloom();
}
- @Override
- public void beforeShipped() throws IOException {
- // For now these writer will always be of type ShipperListener true.
- // TODO : Change all writers to be specifically created for compaction
context
- writer.beforeShipped();
- if (generalBloomFilterWriter != null) {
- generalBloomFilterWriter.beforeShipped();
- }
- if (deleteFamilyBloomFilterWriter != null) {
- deleteFamilyBloomFilterWriter.beforeShipped();
+ /**
+ * For unit testing only.
+ * @return the Bloom filter used by this writer.
+ */
+ BloomFilterWriter getGeneralBloomWriter() {
+ return liveFileWriter.generalBloomFilterWriter;
+ }
+
+ public void close() throws IOException {
+ liveFileWriter.appendFileInfo(HISTORICAL_KEY, Bytes.toBytes(false));
+ liveFileWriter.close();
+ if (historicalFileWriter != null) {
+ historicalFileWriter.appendFileInfo(HISTORICAL_KEY, Bytes.toBytes(true));
+ historicalFileWriter.close();
}
}
- public Path getPath() {
- return this.writer.getPath();
+ public void appendFileInfo(byte[] key, byte[] value) throws IOException {
+ liveFileWriter.appendFileInfo(key, value);
+ if (historicalFileWriter != null) {
+ historicalFileWriter.appendFileInfo(key, value);
+ }
}
- public boolean hasGeneralBloom() {
- return this.generalBloomFilterWriter != null;
+ /**
+ * For use in testing.
+ */
+ HFile.Writer getHFileWriter() {
Review Comment:
Name it getLiveFileWriter?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileManager.java:
##########
@@ -50,15 +50,15 @@ public interface StoreFileManager {
*/
@RestrictedApi(explanation = "Should only be called in StoreEngine", link =
"",
allowedOnPath =
".*(/org/apache/hadoop/hbase/regionserver/StoreEngine.java|/src/test/.*)")
- void loadFiles(List<HStoreFile> storeFiles);
+ void loadFiles(List<HStoreFile> storeFiles) throws IOException;
Review Comment:
Any updates here?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreFileManager.java:
##########
@@ -86,13 +111,20 @@ public Collection<HStoreFile> getCompactedfiles() {
}
@Override
- public void insertNewFiles(Collection<HStoreFile> sfs) {
+ public void insertNewFiles(Collection<HStoreFile> sfs) throws IOException {
+ if (enableLiveFileTracking) {
+ this.liveStoreFiles =
ImmutableList.sortedCopyOf(getStoreFileComparator(),
Review Comment:
So we update liveStoreFiles and storefiles with two statement, will this
cause any consistent problems?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java:
##########
@@ -247,7 +222,10 @@ public void appendMetadata(final long maxSequenceId, final
boolean majorCompacti
* @throws IOException problem writing to FS
*/
public void appendMobMetadata(SetMultimap<TableName, String> mobRefSet)
throws IOException {
- writer.appendFileInfo(MOB_FILE_REFS,
MobUtils.serializeMobFileRefs(mobRefSet));
+ liveFileWriter.appendMobMetadata(mobRefSet);
Review Comment:
Ditto.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java:
##########
@@ -256,156 +234,571 @@ public void appendMobMetadata(SetMultimap<TableName,
String> mobRefSet) throws I
public void appendTrackedTimestampsToMetadata() throws IOException {
// TODO: The StoreFileReader always converts the byte[] to TimeRange
// via TimeRangeTracker, so we should write the serialization data of
TimeRange directly.
- appendFileInfo(TIMERANGE_KEY,
TimeRangeTracker.toByteArray(timeRangeTracker));
- appendFileInfo(EARLIEST_PUT_TS, Bytes.toBytes(earliestPutTs));
+ liveFileWriter.appendTrackedTimestampsToMetadata();
Review Comment:
OK, I think the metadata is a missing part in our previous designs...
In general, we should support writing multiple files for the
DefaultStoreFileWriter, but then, we need to track some metadatas separately,
for the different files, like timestamp range, cells count, etc.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java:
##########
@@ -256,156 +234,571 @@ public void appendMobMetadata(SetMultimap<TableName,
String> mobRefSet) throws I
public void appendTrackedTimestampsToMetadata() throws IOException {
// TODO: The StoreFileReader always converts the byte[] to TimeRange
// via TimeRangeTracker, so we should write the serialization data of
TimeRange directly.
- appendFileInfo(TIMERANGE_KEY,
TimeRangeTracker.toByteArray(timeRangeTracker));
- appendFileInfo(EARLIEST_PUT_TS, Bytes.toBytes(earliestPutTs));
+ liveFileWriter.appendTrackedTimestampsToMetadata();
+ if (historicalFileWriter != null) {
+ historicalFileWriter.appendTrackedTimestampsToMetadata();
+ }
}
/**
* Record the earlest Put timestamp. If the timeRangeTracker is not set,
update TimeRangeTracker
* to include the timestamp of this key
*/
public void trackTimestamps(final Cell cell) {
- if (KeyValue.Type.Put.getCode() == cell.getTypeByte()) {
- earliestPutTs = Math.min(earliestPutTs, cell.getTimestamp());
+ liveFileWriter.trackTimestamps(cell);
Review Comment:
I do not think this is the correct behavior, we should only call the write's
trackTimestamps method if the cell has been written to the writer...
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java:
##########
@@ -256,156 +234,571 @@ public void appendMobMetadata(SetMultimap<TableName,
String> mobRefSet) throws I
public void appendTrackedTimestampsToMetadata() throws IOException {
// TODO: The StoreFileReader always converts the byte[] to TimeRange
// via TimeRangeTracker, so we should write the serialization data of
TimeRange directly.
- appendFileInfo(TIMERANGE_KEY,
TimeRangeTracker.toByteArray(timeRangeTracker));
- appendFileInfo(EARLIEST_PUT_TS, Bytes.toBytes(earliestPutTs));
+ liveFileWriter.appendTrackedTimestampsToMetadata();
+ if (historicalFileWriter != null) {
+ historicalFileWriter.appendTrackedTimestampsToMetadata();
+ }
}
/**
* Record the earlest Put timestamp. If the timeRangeTracker is not set,
update TimeRangeTracker
* to include the timestamp of this key
*/
public void trackTimestamps(final Cell cell) {
- if (KeyValue.Type.Put.getCode() == cell.getTypeByte()) {
- earliestPutTs = Math.min(earliestPutTs, cell.getTimestamp());
+ liveFileWriter.trackTimestamps(cell);
+ if (historicalFileWriter != null) {
+ historicalFileWriter.trackTimestamps(cell);
}
- timeRangeTracker.includeTimestamp(cell);
}
- private void appendGeneralBloomfilter(final Cell cell) throws IOException {
- if (this.generalBloomFilterWriter != null) {
- /*
- *
http://2.bp.blogspot.com/_Cib_A77V54U/StZMrzaKufI/AAAAAAAAADo/ZhK7bGoJdMQ/s400/KeyValue.png
- * Key = RowLen + Row + FamilyLen + Column [Family + Qualifier] +
Timestamp 3 Types of
- * Filtering: 1. Row = Row 2. RowCol = Row + Qualifier 3.
RowPrefixFixedLength = Fixed Length
- * Row Prefix
- */
- bloomContext.writeBloom(cell);
+ @Override
+ public void beforeShipped() throws IOException {
+ liveFileWriter.beforeShipped();
+ if (historicalFileWriter != null) {
+ historicalFileWriter.beforeShipped();
}
}
- private void appendDeleteFamilyBloomFilter(final Cell cell) throws
IOException {
- if (!PrivateCellUtil.isDeleteFamily(cell) &&
!PrivateCellUtil.isDeleteFamilyVersion(cell)) {
- return;
- }
+ public Path getPath() {
+ return liveFileWriter.getPath();
Review Comment:
Maybe we should change this to a List of Paths?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java:
##########
@@ -235,10 +210,10 @@ private byte[]
toCompactionEventTrackerBytes(Collection<HStoreFile> storeFiles)
*/
public void appendMetadata(final long maxSequenceId, final boolean
majorCompaction,
final long mobCellsCount) throws IOException {
- writer.appendFileInfo(MAX_SEQ_ID_KEY, Bytes.toBytes(maxSequenceId));
- writer.appendFileInfo(MAJOR_COMPACTION_KEY,
Bytes.toBytes(majorCompaction));
- writer.appendFileInfo(MOB_CELLS_COUNT, Bytes.toBytes(mobCellsCount));
- appendTrackedTimestampsToMetadata();
+ liveFileWriter.appendMetadata(maxSequenceId, majorCompaction,
mobCellsCount);
Review Comment:
Is this the correct behavior? We should track mobCellsCount for the two
writers separately?
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
