frostruan commented on a change in pull request #3261:
URL: https://github.com/apache/hbase/pull/3261#discussion_r632560241
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -1953,6 +1954,24 @@ private void addToCompactingFiles(Collection<HStoreFile>
filesToAdd) {
Collections.sort(filesCompacting,
storeEngine.getStoreFileManager().getStoreFileComparator());
}
+ /**
+ * Remove the files from compacting files. This usually happens when we
clear compaction queues.
+ */
+ void removeFromCompactingFiles(Collection<HStoreFile> filesToRemove) {
+ synchronized (filesCompacting) {
+ filesCompacting.removeAll(filesToRemove);
+ Collections.sort(filesCompacting,
storeEngine.getStoreFileManager().getStoreFileComparator());
+ }
Review comment:
my bad. I'll fix it.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -1953,6 +1954,24 @@ private void addToCompactingFiles(Collection<HStoreFile>
filesToAdd) {
Collections.sort(filesCompacting,
storeEngine.getStoreFileManager().getStoreFileComparator());
}
+ /**
+ * Remove the files from compacting files. This usually happens when we
clear compaction queues.
+ */
+ void removeFromCompactingFiles(Collection<HStoreFile> filesToRemove) {
+ synchronized (filesCompacting) {
+ filesCompacting.removeAll(filesToRemove);
+ Collections.sort(filesCompacting,
storeEngine.getStoreFileManager().getStoreFileComparator());
+ }
Review comment:
yes. I have removed unnecessary sorting. Thanks for reviewing.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java
##########
@@ -794,13 +795,30 @@ void shutdownLongCompactions(){
}
public void clearLongCompactionsQueue() {
+ removeFromFilesCompacting(longCompactions);
longCompactions.getQueue().clear();
}
public void clearShortCompactionsQueue() {
+ removeFromFilesCompacting(shortCompactions);
shortCompactions.getQueue().clear();
}
+ private void removeFromFilesCompacting(ThreadPoolExecutor compactor) {
+ for (Runnable runnable : compactor.getQueue()) {
+ if (!(runnable instanceof CompactionRunner)) {
+ continue;
+ }
+ CompactionRunner runner = (CompactionRunner) runnable;
+ // for system compaction, files selection will be delayed until the
compaction task
+ // actually runs, so compaction context is null for system compaction
+ if (runner.compaction != null) {
+ Collection<HStoreFile> files =
runner.compaction.getRequest().getFiles();
Review comment:
yes. I added a check to make sure compaction has selection. Thanks.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java
##########
@@ -794,13 +795,30 @@ void shutdownLongCompactions(){
}
public void clearLongCompactionsQueue() {
+ removeFromFilesCompacting(longCompactions);
longCompactions.getQueue().clear();
}
public void clearShortCompactionsQueue() {
+ removeFromFilesCompacting(shortCompactions);
Review comment:
I have fixed it, thanks.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java
##########
@@ -794,13 +795,30 @@ void shutdownLongCompactions(){
}
public void clearLongCompactionsQueue() {
+ removeFromFilesCompacting(longCompactions);
Review comment:
yes. This is done. Thanks.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java
##########
@@ -794,11 +795,27 @@ void shutdownLongCompactions(){
}
public void clearLongCompactionsQueue() {
- longCompactions.getQueue().clear();
+ removeFromFilesCompacting(longCompactions);
}
public void clearShortCompactionsQueue() {
- shortCompactions.getQueue().clear();
+ removeFromFilesCompacting(shortCompactions);
+ }
+
+ private void removeFromFilesCompacting(ThreadPoolExecutor compactor) {
+ Iterator<Runnable> iter = compactor.getQueue().iterator();
+ while (iter.hasNext()) {
+ Runnable runnable = iter.next();
+ if (!(runnable instanceof CompactionRunner)) {
+ continue;
+ }
+ CompactionRunner runner = (CompactionRunner) runnable;
+ if (runner.compaction != null && runner.compaction.hasSelection()) {
Review comment:
Thanks for taking a look on this PR.
I don't think there is data race here. It's safe. Thanks.
--
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:
[email protected]
