HBASE-28956 RSMobFileCleanerChore may close the StoreFileReader object which is being used by Compaction thread #6439
Conversation
| if (sf.getReader() == null) { | ||
| synchronized (sf) { | ||
| if (sf.getReader() == null) { | ||
| needCreateReader = true; | ||
| sf.initReader(); | ||
| mobRefData = sf.getMetadataValue(HStoreFile.MOB_FILE_REFS); | ||
| bulkloadMarkerData = sf.getMetadataValue(HStoreFile.BULKLOAD_TASK_KEY); | ||
| // close store file to avoid memory leaks | ||
| sf.closeStoreFile(true); | ||
| } | ||
| } | ||
| } | ||
| // If the StoreFileReader object was created by another thread, even if the reader | ||
| // has been closed now, we can still obtain the data by HStoreFile.metataMap, | ||
| // because the map will not be cleared when the reader is closed. | ||
| if (!needCreateReader) { | ||
| mobRefData = sf.getMetadataValue(HStoreFile.MOB_FILE_REFS); | ||
| bulkloadMarkerData = sf.getMetadataValue(HStoreFile.BULKLOAD_TASK_KEY); | ||
| } |
There was a problem hiding this comment.
Could we save some lines here? Seems we only need to avoid the sf.closeStoreFile(true); if the reader was not yet null, so maybe just the below would do:
boolean needCreateReader = sf.getReader() == null;
sf.initReader();
byte[] mobRefData = sf.getMetadataValue(HStoreFile.MOB_FILE_REFS);
byte[] bulkloadMarkerData = sf.getMetadataValue(HStoreFile.BULKLOAD_TASK_KEY);
// close store file to avoid memory leaks
if(needCreateReader) {
sf.closeStoreFile(true);
}
There was a problem hiding this comment.
Thanks for your review.
I think this way may still have the same problem.
Assume that there are two threads holding the same StoreFile object.
Let's take a look at this situation:
// Time1 RSMobFileCleanerChore (Thread A) executing this code, and reader is null
// now needCreateReader is true
boolean needCreateReader = sf.getReader() == null;
// Time2 another thread (Thread B) create the reader object
// Time3 Thread A continue to execute the code
// now reader has been created by Thread B,
// so RSMobFileCleanerChore will not create the reader object
sf.initReader();
byte[] mobRefData = sf.getMetadataValue(HStoreFile.MOB_FILE_REFS);
byte[] bulkloadMarkerData = sf.getMetadataValue(HStoreFile.BULKLOAD_TASK_KEY);
// Time4 RSMobFileCleanerChore continue to execute the code
// because needCreateReader is true, so it will close the reader which isn't created by itself
if(needCreateReader) {
sf.closeStoreFile(true);
}There was a problem hiding this comment.
Put it under synchronized ?
There was a problem hiding this comment.
Indeed, writing code this way will be much more concise.
But at the same time, I think performance also needs to be considered.
When the reader does not need to be created by itself, We do not need to perform read operations in the synchronization code block because locking may cause other threads to wait. In this case, exiting the synchronization code block early may reduce the waiting time for other threads. （If there are other threads waiting to obtain this lock）
Therefore, when executing the following code, I think it needs to be divided into two cases:
if (sf.getReader() == null) {
synchronized (sf) {
boolean needCreateReader = sf.getReader() == null;
// Case 1: needCreateReader = true, create reader, and do something
// Case 2: needCreateReader = false, exit the synchronization code block to reduce performance overhead
}
}What do you think?
I look forward to your response. Thanks a lot!
There was a problem hiding this comment.
I guess the operations within the synchronized block are all O(1), apart from close when we have to evict blocks from the cache, but we would only call close here if the reader was null, which means, this file has probably been compacted already and have no references to it. Now, in such cases when reader was null, would we ever open a reader again outside this cleaner chore? I suppose not.
Nevertheless, this is a minor style suggestion, am ok to go without it.
There was a problem hiding this comment.
Yes, agree with you, Let me make some changes later.
Thank you very much for your patient and detailed review!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…eing used by Compaction thread
guluo2016
force-pushed
the
hbase_HBASE-28956
branch
from
1b3e4e4
to
3cd6d64
Compare
|
The change has been made. thanks. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Details see: HBASE-28956