Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
SwaraliJoshi commented on code in PR #7552:
URL: https://github.com/apache/hbase/pull/7552#discussion_r3365919869
##
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java:
##
@@ -1775,6 +1776,204 @@ public void testReclaimChunkWhenScaning() throws
IOException {
}
}
+ @Test
+ public void testReaderRaceConditionDuringCompaction() throws Exception {
+HBaseTestingUtil util = new HBaseTestingUtil();
+util.startMiniCluster(1);
+
+try {
+ Configuration conf = util.getConfiguration();
+
+ byte[] FAMILY = Bytes.toBytes("f");
+ byte[] Q = Bytes.toBytes("q");
+ TableName TABLE = TableName.valueOf("race_test");
+
+ TableDescriptor td = TableDescriptorBuilder.newBuilder(TABLE)
+.setColumnFamily(
+ ColumnFamilyDescriptorBuilder.newBuilder(FAMILY)
+.setMaxVersions(1)
+.build())
+.build();
+
+ util.getAdmin().createTable(td);
+
+ HRegion region =
+util.getMiniHBaseCluster().getRegions(TABLE).get(0);
+
+ HStore store = region.getStore(FAMILY);
+
+ for (int i = 0; i < 4; i++) {
+Put p = new Put(Bytes.toBytes("row-" + i));
+p.addColumn(FAMILY, Q, Bytes.toBytes(i));
+region.put(p);
+region.flush(true); // force store file
+ }
+
+ // sanity check
+ assertEquals(4, store.getStorefilesCount());
+
+ DefaultStoreFileManager sfm =
+(DefaultStoreFileManager) store.getStoreEngine().getStoreFileManager();
+
+ HStoreFile victim =
+sfm.getStoreFiles().iterator().next();
+
+ AtomicReference failure = new AtomicReference<>();
+ CountDownLatch start = new CountDownLatch(1);
+ CountDownLatch done = new CountDownLatch(2);
+
+ Thread remover = new Thread(() -> {
+try {
+ start.await();
+ victim.closeStoreFile(true); // async-style close
Review Comment:
The discharger, while archiving a compacted file, reaches closeStoreFile
through this chain:
`HStore.removeCompactedfiles` →
`storeFileTracker.removeStoreFiles(filesToRemove)` (HStore.java:2333) →
`HFileArchiver.archiveStoreFiles` → `resolveAndArchiveFile` →
`FileableStoreFile.moveAndClose` → `this.close()`:
```
HFileArchiver.java
Lines 887-890
public void close() throws IOException {
file.closeStoreFile(true); // -> HStoreFile.closeStoreFile ->
initialReader = null
}
```
This nulling runs under **archiveLock** only — not under
storeEngine.writeLock — and it happens at line 2333, before clearCompactedfiles
(line 2357) removes the file from the compactedfiles list. (Note: the
r.close(evictOnClose) at line 2306 closes the reader object but operates on a
local variable; it does not null the field. The field nulling is only the
closeStoreFile above.)
Now during this time if addCompactionResults is running, we see NPE because
compaction/selection thread (HStore.compact/removeUnneededFiles →
`replaceStoreFiles` → `StoreEngine.replaceStoreFiles` →
`addCompactionResults`): runs under storeEngine.writeLock and sorts
Iterables.concat(compactedfiles, newCompactedfiles).
--
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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
SwaraliJoshi commented on code in PR #7552:
URL: https://github.com/apache/hbase/pull/7552#discussion_r3353865603
##
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreFileManager.java:
##
@@ -179,9 +179,30 @@ public void addCompactionResults(Collection
newCompactedfiles,
// Let a background thread close the actual reader on these compacted
files and also
// ensure to evict the blocks from block cache so that they are no longer
in
// cache
-newCompactedfiles.forEach(HStoreFile::markCompactedAway);
-compactedfiles = ImmutableList.sortedCopyOf(storeFileComparator,
- Iterables.concat(compactedfiles, newCompactedfiles));
+List filesToClose = new ArrayList<>(newCompactedfiles);
+try {
+ HStoreFile.increaseStoreFilesRefeCount(newCompactedfiles);
+ newCompactedfiles.forEach(hStoreFile -> {
+StoreFileReader reader = hStoreFile.getReader();
+try {
+ if (reader == null) {
+hStoreFile.initReader();
+ } else {
+filesToClose.remove(hStoreFile);
+ }
+} catch (IOException e) {
+ LOG.warn("Couldn't initialize reader for " + hStoreFile, e);
+ throw new RuntimeException(e);
Review Comment:
Yes, I will make the change. We are calling initReader here as the reader is
null and we need to access reader.getMaximumTimestamp(). This is the cause of
NPE which we are trying to fix.
--
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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
SwaraliJoshi commented on PR #7552: URL: https://github.com/apache/hbase/pull/7552#issuecomment-4619459461 > This only affects DateTieredCompaction or is a general problem for all compaction algorithm? @Apache9 This only happens for DateTieredCompaction as it uses getMaximumTimestamp(). Other compactions are not affected and would not face this issue. -- 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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
Apache9 commented on code in PR #7552:
URL: https://github.com/apache/hbase/pull/7552#discussion_r3295705369
##
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java:
##
@@ -1775,6 +1776,204 @@ public void testReclaimChunkWhenScaning() throws
IOException {
}
}
+ @Test
+ public void testReaderRaceConditionDuringCompaction() throws Exception {
+HBaseTestingUtil util = new HBaseTestingUtil();
+util.startMiniCluster(1);
+
+try {
+ Configuration conf = util.getConfiguration();
+
+ byte[] FAMILY = Bytes.toBytes("f");
+ byte[] Q = Bytes.toBytes("q");
+ TableName TABLE = TableName.valueOf("race_test");
+
+ TableDescriptor td = TableDescriptorBuilder.newBuilder(TABLE)
+.setColumnFamily(
+ ColumnFamilyDescriptorBuilder.newBuilder(FAMILY)
+.setMaxVersions(1)
+.build())
+.build();
+
+ util.getAdmin().createTable(td);
+
+ HRegion region =
+util.getMiniHBaseCluster().getRegions(TABLE).get(0);
+
+ HStore store = region.getStore(FAMILY);
+
+ for (int i = 0; i < 4; i++) {
+Put p = new Put(Bytes.toBytes("row-" + i));
+p.addColumn(FAMILY, Q, Bytes.toBytes(i));
+region.put(p);
+region.flush(true); // force store file
+ }
+
+ // sanity check
+ assertEquals(4, store.getStorefilesCount());
+
+ DefaultStoreFileManager sfm =
+(DefaultStoreFileManager) store.getStoreEngine().getStoreFileManager();
+
+ HStoreFile victim =
+sfm.getStoreFiles().iterator().next();
+
+ AtomicReference failure = new AtomicReference<>();
+ CountDownLatch start = new CountDownLatch(1);
+ CountDownLatch done = new CountDownLatch(2);
+
+ Thread remover = new Thread(() -> {
+try {
+ start.await();
+ victim.closeStoreFile(true); // async-style close
Review Comment:
So how could this happen in real production?
##
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java:
##
@@ -1775,6 +1776,204 @@ public void testReclaimChunkWhenScaning() throws
IOException {
}
}
+ @Test
+ public void testReaderRaceConditionDuringCompaction() throws Exception {
+HBaseTestingUtil util = new HBaseTestingUtil();
Review Comment:
Better introduce a new test class if we need start a mini cluster here.
##
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreFileManager.java:
##
@@ -179,9 +179,30 @@ public void addCompactionResults(Collection
newCompactedfiles,
// Let a background thread close the actual reader on these compacted
files and also
// ensure to evict the blocks from block cache so that they are no longer
in
// cache
-newCompactedfiles.forEach(HStoreFile::markCompactedAway);
-compactedfiles = ImmutableList.sortedCopyOf(storeFileComparator,
- Iterables.concat(compactedfiles, newCompactedfiles));
+List filesToClose = new ArrayList<>(newCompactedfiles);
+try {
+ HStoreFile.increaseStoreFilesRefeCount(newCompactedfiles);
+ newCompactedfiles.forEach(hStoreFile -> {
+StoreFileReader reader = hStoreFile.getReader();
+try {
+ if (reader == null) {
+hStoreFile.initReader();
+ } else {
+filesToClose.remove(hStoreFile);
+ }
+} catch (IOException e) {
+ LOG.warn("Couldn't initialize reader for " + hStoreFile, e);
+ throw new RuntimeException(e);
Review Comment:
Just use a for loop and throw the IOException out directly?
And please add comments to describe the logic here? Why we need to call
initReader here?
--
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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
Apache9 commented on PR #7552: URL: https://github.com/apache/hbase/pull/7552#issuecomment-4530862677 > @Apache9 My sincere apologies for delayed response. Here are the details : > > > but it is still a bit strange to me that why we close a store file reader and then we still try to select for compaction? > > So here we are actually seeing that the file is sent for compaction (DateTieredCompaction) and simultaneously when the file is marked as compacted, the reader is closed > > _newCompactedfiles.forEach(HStoreFile::markCompactedAway);_ > > Here we are marking them compacted while a parallel thread is marking all the marked files closed. Whereas in the next step we have : > > _compactedfiles = ImmutableList.sortedCopyOf(storeFileComparator, Iterables.concat(compactedfiles, newCompactedfiles));_ > > Here the sorting happens based on timestamp (_reader.getMaximumTimestamp()_) where we are seeing the NPE. > > The root cause of the NPE is a race condition between store file reader closure and compaction bookkeeping. During compaction, the system performs two logically separate steps: > > * Compaction bookkeeping > The StoreFileManager (e.g. DefaultStoreFileManager) removes compacted store files from its internal active file set via addCompactionResults(...). > * Reader cleanup > The compacted HStoreFile’s reader is eventually closed (via closeStoreFile(...)), either synchronously or asynchronously. > > Before this fix, these two steps were not strictly ordered. As a result, it was possible for a store file’s reader to be closed while the file was still visible to compaction logic. This only affects DateTieredCompaction or is a general problem for all compaction algorithm? -- 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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
SwaraliJoshi commented on PR #7552: URL: https://github.com/apache/hbase/pull/7552#issuecomment-4530492466 @Apache9 Kind reminder for review. Please help here. -- 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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
SwaraliJoshi commented on PR #7552: URL: https://github.com/apache/hbase/pull/7552#issuecomment-4448495319 @Apache9 Gentle reminder for review on this PR -- 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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
SwaraliJoshi commented on PR #7552: URL: https://github.com/apache/hbase/pull/7552#issuecomment-4326850766 @Apache9 Kindly requesting you to please review this PR. I would be more than happy to connect to discuss about the fix further 😃 -- 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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
SwaraliJoshi commented on PR #7552: URL: https://github.com/apache/hbase/pull/7552#issuecomment-4249390055 >but it is still a bit strange to me that why we close a store file reader and then we still try to select for compaction? So here we are actually seeing that the file is sent for compaction (DateTieredCompaction) and simultaneously when the file is marked as compacted, the reader is closed *newCompactedfiles.forEach(HStoreFile::markCompactedAway);* Here we are marking them compacted while a parallel thread is marking all the marked files closed. Whereas in the next step we have : *compactedfiles = ImmutableList.sortedCopyOf(storeFileComparator, Iterables.concat(compactedfiles, newCompactedfiles));* Here the sorting happens based on timestamp (*reader.getMaximumTimestamp()*) where we are seeing the NPE. The root cause of the NPE is a race condition between store file reader closure and compaction bookkeeping. During compaction, the system performs two logically separate steps: * Compaction bookkeeping The StoreFileManager (e.g. DefaultStoreFileManager) removes compacted store files from its internal active file set via addCompactionResults(...). * Reader cleanup The compacted HStoreFile’s reader is eventually closed (via closeStoreFile(...)), either synchronously or asynchronously. Before this fix, these two steps were not strictly ordered. As a result, it was possible for a store file’s reader to be closed while the file was still visible to compaction logic. -- 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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
Apache9 commented on PR #7552: URL: https://github.com/apache/hbase/pull/7552#issuecomment-3703829944 Thanks for providing the details, but it is still a bit strange to me that why we close a store file reader and then we still try to select for compaction? And seems the code changes are mostly in addCompactionResult, in general, we will only close a store file reader after it is compacted away, and there should be no overlap on store files between different compactions, then why do we want to close a store file while it is stil referenced in a compaction? 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
SwaraliJoshi commented on PR #7552: URL: https://github.com/apache/hbase/pull/7552#issuecomment-3701412788 @Apache9 I have updated the description with all the details now. Kindly take a look please. In short, NPE was caused by a race where a store file reader could be closed before DateTieredCompaction finished using it and we are trying to fix the same here. -- 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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
Apache-HBase commented on PR #7552:
URL: https://github.com/apache/hbase/pull/7552#issuecomment-3700868778
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|::|--:|:|::|:---:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | | Unprocessed flag(s):
--brief-report-file --spotbugs-strict-precheck --author-ignore-list
--blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck |
_ Prechecks _ |
_ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 37s | | master passed |
| +1 :green_heart: | compile | 1m 0s | | master passed |
| +1 :green_heart: | javadoc | 0m 30s | | master passed |
| +1 :green_heart: | shadedjars | 6m 16s | | branch has no errors when
building our shaded downstream artifacts. |
_ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 6s | | the patch passed |
| +1 :green_heart: | compile | 1m 0s | | the patch passed |
| +1 :green_heart: | javac | 1m 0s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 27s | | the patch passed |
| +1 :green_heart: | shadedjars | 6m 10s | | patch has no errors when
building our shaded downstream artifacts. |
_ Other Tests _ |
| -1 :x: | unit | 228m 57s |
[/patch-unit-hbase-server.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7552/2/artifact/yetus-jdk17-hadoop3-check/output/patch-unit-hbase-server.txt)
| hbase-server in the patch failed. |
| | | 256m 26s | | |
| Subsystem | Report/Notes |
|--:|:-|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base:
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7552/2/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
|
| GITHUB PR | https://github.com/apache/hbase/pull/7552 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux ae869da82352 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May
23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 171ba9928ce97b85dae6749ea179c2daf2491900 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results |
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7552/2/testReport/
|
| Max. process+thread count | 4846 (vs. ulimit of 3) |
| modules | C: hbase-server U: hbase-server |
| Console output |
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7552/2/console
|
| versions | git=2.34.1 maven=3.9.8 |
| Powered by | Apache Yetus 0.15.0 https://yetus.apache.org |
This message was automatically generated.
--
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]
Re: [PR] HBASE-29348 Fixed NPE issue due to HStoreFile getting closed during race condition [hbase]
Apache-HBase commented on PR #7552:
URL: https://github.com/apache/hbase/pull/7552#issuecomment-3700401200
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|::|--:|:|::|:---:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
_ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files
found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available.
|
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain
any @author tags. |
| +1 :green_heart: | hbaseanti | 0m 0s | | Patch does not have any
anti-patterns. |
_ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 21s | | master passed |
| +1 :green_heart: | compile | 3m 16s | | master passed |
| +1 :green_heart: | checkstyle | 0m 58s | | master passed |
| +1 :green_heart: | spotbugs | 1m 34s | | master passed |
| +1 :green_heart: | spotless | 0m 49s | | branch has no errors when
running spotless:check. |
_ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 54s | | the patch passed |
| +1 :green_heart: | compile | 3m 15s | | the patch passed |
| +1 :green_heart: | javac | 3m 15s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks
issues. |
| +1 :green_heart: | checkstyle | 0m 57s | | the patch passed |
| +1 :green_heart: | spotbugs | 1m 39s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 11m 17s | | Patch does not cause any
errors with Hadoop 3.3.6 3.4.1. |
| -1 :x: | spotless | 0m 36s | | patch has 63 errors when running
spotless:check, run spotless:apply to fix. |
_ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 12s | | The patch does not
generate ASF License warnings. |
| | | 38m 33s | | |
| Subsystem | Report/Notes |
|--:|:-|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base:
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7552/2/artifact/yetus-general-check/output/Dockerfile
|
| GITHUB PR | https://github.com/apache/hbase/pull/7552 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell
detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux 6432dc50d6bc 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May
23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 171ba9928ce97b85dae6749ea179c2daf2491900 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| spotless |
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7552/2/artifact/yetus-general-check/output/patch-spotless.txt
|
| Max. process+thread count | 84 (vs. ulimit of 3) |
| modules | C: hbase-server U: hbase-server |
| Console output |
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7552/2/console
|
| versions | git=2.34.1 maven=3.9.8 spotbugs=4.7.3 |
| Powered by | Apache Yetus 0.15.0 https://yetus.apache.org |
This message was automatically generated.
--
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]
