ndimiduk commented on code in PR #6700:
URL: https://github.com/apache/hbase/pull/6700#discussion_r1958484159
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java:
##########
@@ -1132,19 +1132,20 @@ public void
deleteChangedReaderObserver(ChangedReadersObserver o) {
* <p>
* Compaction event should be idempotent, since there is no IO Fencing for
the region directory in
* hdfs. A region server might still try to complete the compaction after it
lost the region. That
- * is why the following events are carefully ordered for a compaction: 1.
Compaction writes new
Review Comment:
Same with this comment.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -2332,37 +2332,76 @@ private boolean shouldForbidMajorCompaction() {
}
/**
- * We are trying to remove / relax the region read lock for compaction.
Let's see what are the
Review Comment:
This comment was helpful for understanding the compaction logic. I cleaned
up the formatting to how it was before spotless, using proper javadoc syntax.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java:
##########
@@ -202,7 +205,7 @@ protected final void createComponentsOnce(Configuration
conf, HStore store,
this.openStoreFileThreadPoolCreator =
store.getHRegion()::getStoreFileOpenAndCloseThreadPool;
this.storeFileTracker = createStoreFileTracker(conf, store);
assert compactor != null && compactionPolicy != null && storeFileManager
!= null
- && storeFlusher != null && storeFileTracker != null;
Review Comment:
Static analysis says `storeFileTracker` is always null.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java:
##########
@@ -229,12 +232,23 @@ public HStoreFile createStoreFileAndReader(StoreFileInfo
info) throws IOExceptio
/**
* Validates a store file by opening and closing it. In HFileV2 this should
not be an expensive
* operation.
- * @param path the path to the store file
+ * @param path the path to the store file
+ * @param isCompaction whether this is called from the context of a
compaction
*/
- public void validateStoreFile(Path path) throws IOException {
+ public void validateStoreFile(Path path, boolean isCompaction) throws
IOException {
HStoreFile storeFile = null;
try {
storeFile = createStoreFileAndReader(path);
+ if (conf.getBoolean(READ_FULLY_ON_VALIDATE_KEY,
DEFAULT_READ_FULLY_ON_VALIDATE)) {
Review Comment:
This is the core of the new feature. Anywhere that we previously would
invoke `validateStoreFile()` will now conditionally include the more rigorous
check. There are a couple places where we do NOT call `validateStoreFile` after
writing it, I'm not yet sure why.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
