HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode #528
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ZhaoBQ
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Reidddddd
reviewed
Aug 30, 2019
| @@ -242,6 +244,11 @@ public int compare(BlockCacheKey a, BlockCacheKey b) { | |||
| /** In-memory bucket size */ | |||
| private float memoryFactor; | |||
|
|
|||
| private String[] filePaths; | |||
| static final String FILE_VERIFY_ALGORITHM = "hbase.bucketcache.file.verify.algorithm"; | |||
| static final String DEFAULT_FILE_VERIFY_ALGORITHM = "MD5"; | |||
There was a problem hiding this comment.
Set them private, unless you use them somewhere outside package.
Reidddddd
reviewed
Aug 30, 2019
| @@ -288,14 +294,15 @@ public BucketCache(String ioEngineName, long capacity, int blockSize, int[] buck | |||
| this.ramCache = new ConcurrentHashMap<BlockCacheKey, RAMQueueEntry>(); | |||
|
|
|||
| this.backingMap = new ConcurrentHashMap<BlockCacheKey, BucketEntry>((int) blockNumCapacity); | |||
|
|
|||
| this.algorithm = conf.get(FILE_VERIFY_ALGORITHM,DEFAULT_FILE_VERIFY_ALGORITHM); | |||
There was a problem hiding this comment.
nit, space ,DEFAULT_FILE_VERIFY_ALGORITHM
Reidddddd
reviewed
Aug 30, 2019
| } catch (NoSuchAlgorithmException e) { | ||
| LOG.error("No such algorithm : " + algorithm + "! Failed to persist data on exit",e); | ||
| } catch (Exception e) { | ||
| LOG.error("persist to file error"+e); |
Reidddddd
reviewed
Aug 30, 2019
| @@ -1056,6 +1069,7 @@ private void retrieveFromFile(int[] bucketSizes) throws IOException, BucketAlloc | |||
| "Attempt to restore non-persistent cache mappings!"); | |||
| fis = new FileInputStream(persistencePath); | |||
| ois = new ObjectInputStream(fis); | |||
| ois.readUTF(); | |||
There was a problem hiding this comment.
It breaks backward compatibility. For an upgrading rs, it should read an old persistent file.
Reidddddd
reviewed
Aug 30, 2019
| public static String getPreFilesKey(String persistencePath) throws IOException { | ||
| FileInputStream fis = null; | ||
| ObjectInputStream ois = null; | ||
| try { |
There was a problem hiding this comment.
Use try-with-resource style
Reidddddd
reviewed
Aug 30, 2019
| init(); | ||
| } | ||
|
|
||
| public void init() throws IOException { |
There was a problem hiding this comment.
this method should be private?
Reidddddd
reviewed
Aug 30, 2019
| import java.security.MessageDigest; | ||
| import java.security.NoSuchAlgorithmException; | ||
|
|
||
| public class FileIOEngineUtils { |
Reidddddd
reviewed
Aug 30, 2019
| public class FileIOEngineUtils { | ||
|
|
||
| private FileIOEngineUtils() { | ||
|
|
Reidddddd
reviewed
Aug 30, 2019
| private FileIOEngineUtils() { | ||
|
|
||
| } | ||
| /** |
There was a problem hiding this comment.
There should be a new line.
Reidddddd
reviewed
Aug 30, 2019
| * @throws IOException something happened like file not exists | ||
| * @throws NoSuchAlgorithmException no such algorithm | ||
| */ | ||
| public static String getFilesKey(String[] filePaths, String algorithmName) throws IOException, NoSuchAlgorithmException { |
There was a problem hiding this comment.
This line is too long to match code style.
Reidddddd
reviewed
Aug 30, 2019
| private String[] filePaths; | ||
| static final String FILE_VERIFY_ALGORITHM = "hbase.bucketcache.file.verify.algorithm"; | ||
| static final String DEFAULT_FILE_VERIFY_ALGORITHM = "MD5"; | ||
| private String algorithm; |
There was a problem hiding this comment.
Java doc for this new member, what is this algorithm for?
Reidddddd
reviewed
Aug 30, 2019
| @@ -242,6 +244,11 @@ public int compare(BlockCacheKey a, BlockCacheKey b) { | |||
| /** In-memory bucket size */ | |||
| private float memoryFactor; | |||
|
|
|||
| private String[] filePaths; | |||
| static final String FILE_VERIFY_ALGORITHM = "hbase.bucketcache.file.verify.algorithm"; | |||
There was a problem hiding this comment.
Please rename this parameters, "hbase.bucketcache.persistent.file.integrity.check.algorithm" for your reference.
Reidddddd
requested changes
Aug 30, 2019
Reidddddd
approved these changes
Sep 11, 2019
|
💔 -1 overall
This message was automatically generated. |
ramkrish86
reviewed
Sep 12, 2019
| @@ -267,6 +318,98 @@ void refreshFileConnection(int accessFileNum, IOException ioe) throws IOExceptio | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public byte[] readPersistenceChecksum(String persistencePath) { | |||
There was a problem hiding this comment.
this can be called readPersistentCheckSum(). But do we need this API. Instead can we just do this internally under verifyCheckSum() or verifyFileIntegrity()?
There was a problem hiding this comment.
It has been modified. Thanks for giving advice.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
saintstack
approved these changes
Sep 14, 2019
There was a problem hiding this comment.
Skimmed. LGTM. @ramkrish86 and @anoopsjohn ... is it good by you fellows? I see @Reidddddd gave it his blessing already.
| private final String[] filePaths; | ||
| private final FileChannel[] fileChannels; | ||
| private final RandomAccessFile[] rafs; | ||
| private final ReentrantLock[] channelLocks; | ||
|
|
||
| private final long sizePerFile; | ||
| private final long capacity; | ||
| private final String algorithmName; | ||
| private boolean isOldVersion; |
There was a problem hiding this comment.
s/isOldVersion/oldVersion/
isOldVersion is name you'd use for the method that returns this boolean.
|
|
||
| /** | ||
| * Whether the persistence file is old version, it's for back compatibility | ||
| * @return true if the persistence file is old version |
There was a problem hiding this comment.
Might say what 'old version' means... means it does not support this feature.
There was a problem hiding this comment.
It has been modified. Thanks!
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Reidddddd
reviewed
Sep 16, 2019
| private static final Log LOG = LogFactory.getLog(FileIOEngine.class); | ||
| public static final String FILE_DELIMITER = ","; | ||
| private static final DuFileCommand du = new DuFileCommand(new String[] {"du", ""}); |
There was a problem hiding this comment.
Upper case for static final member.
Reidddddd
reviewed
Sep 16, 2019
| @Override | ||
| public byte[] calculateChecksum() | ||
| throws IOException, NoSuchAlgorithmException { | ||
|
|
|
💔 -1 overall
This message was automatically generated. |
anoopsjohn
reviewed
Sep 16, 2019
| if (!ioEngine.isPersistent()) { | ||
| throw new IOException("Attempt to persist non-persistent cache mappings!"); | ||
| } | ||
| fos = new FileOutputStream(persistencePath, false); | ||
| oos = new ObjectOutputStream(fos); | ||
| if (ioEngine instanceof PersistentIOEngine) { |
There was a problem hiding this comment.
If we are in this persistToFile() , it means it is PersistentIOEngine. May be an assert and direct casting is better way than if check.
anoopsjohn
reviewed
Sep 16, 2019
| fis = new FileInputStream(persistencePath); | ||
| ois = new ObjectInputStream(fis); | ||
| // for backward compatibility | ||
| if (ioEngine instanceof PersistentIOEngine && |
There was a problem hiding this comment.
Same as above comment. See above
if (!ioEngine.isPersistent()) throw new IOException().
Just after that line itself you can do the typecast.
anoopsjohn
reviewed
Sep 16, 2019
| @@ -1078,9 +1097,8 @@ private void retrieveFromFile(int[] bucketSizes) throws IOException, BucketAlloc | |||
| bucketAllocator = allocator; | |||
| deserialiserMap = deserMap; | |||
| backingMap = backingMapFromFile; | |||
| blockNumber.set(backingMap.size()); | |||
There was a problem hiding this comment.
Why this change? Is it related to this jira directly?
There was a problem hiding this comment.
When retrieve successfully from file, the "Block Count" in WebUI would be 0 if blockNumber is not changed. But it's have blocks actually.
There was a problem hiding this comment.
I see. So this is an existing bug. Its a one liner change. Still can be done as another bug jira may be.
anoopsjohn
reviewed
Sep 16, 2019
| ois.read(PBMagic); | ||
| int length = ois.readInt(); | ||
| byte[] persistenceChecksum = new byte[length]; | ||
| ois.read(persistenceChecksum); |
There was a problem hiding this comment.
Actually we are reading persistentChecksum twice in this flow. At FileIOE create time as part of verify call and here too. Here we are doing as a skip way. So why can't we do it here only? We have verifyFileIntegrity() in PersistentIOEngine interface and we can call that from here? It looks bit odd. The oldVersion check can be done here also based on he PBMagic matching.
isOldVersion() API itself not needed in the FileIOE. We process the persisted meta info here and based on that recreate the backingMap etc here in BC. So knowing whether checksum also persisted and if so verify that all can be here. I mean the actual verify imp can be in FileIOE but the call to that should be from here.
There was a problem hiding this comment.
I thought about your thoughts. If we want to read it once, we should pass ObjectInputStream object "ois" to the verifyFileIntegrity() method. If it's an old version persistent file, the ois object should be reset, but reset() method is not support. We can recreate an ObjectInputStream without using try-with-resource statement, but this may be a bit unsightly......
There was a problem hiding this comment.
May be we need to close and reopen for the read. My biggest worry is where we do this verify. See my below comment. Now we are doing it while creation of FileIOE. If the verify fails, we are not allowing the FileIOE to be created and do its future work. My point is this. We should create the FileIOE. And then the Bucket Cache is trying to retrieve the already cached data from persistent store and for that its recreating the cache meta. At that step we should be doing the verification right. First see whether the checksum for verify is present already and if so verify. If verify ok and then try to recreate the cache meta data. Or else just forget abt that existing persisted cache data and may be do the necessary cleanup. All these work of Bucket Cache. It can ask the FileIOE to do actual verify. But should be initiated by the BucketCache. You get my mind clearly now? Sorry for not saying in detail at 1st step itself.
Ya may be we need close the file and reopen in case of old style with no pb magic. Or else consider it as 4 bytes and read next 4 bytes and make out the 8 bytes long number. But that may be having challenges wrt the platform. I dont think it is an issue to just reopen the file if no pbmagic. Comments. @Reidddddd
There was a problem hiding this comment.
I got your point already. In fact, if the verification fails, FileIOE can still be created. If verification fails, we would throw IOException, then cache the IOException and do some cleanup, but the creation of FileIOE will continue. Below is the code for the cache：
catch (IOException ioex) { LOG.error("File verification failed because of ", ioex); // delete cache files and backingMap persistent file. deleteCacheDataFile(); new File(persistentPath).delete(); }
However, I totally agree with what you said, I will modify it immediately.
anoopsjohn
reviewed
Sep 16, 2019
| if (!Bytes.equals(persistentChecksum, calculateChecksum)) { | ||
| LOG.warn("The persistent checksum is " + Bytes.toString(persistentChecksum) + | ||
| ", but the calculate checksum is " + Bytes.toString(calculateChecksum)); | ||
| throw new IOException(); |
There was a problem hiding this comment.
Actually if the checksum do not match, we can still continue with RS operation. We can not regain the old cached data. But now as this throw IOE happens while construction of the FileIOEngine, we can no longer use the IOEngine itself. That is wrong. One more reason not to do this verify as part of constructor but at a later time as part of retrieve from persisted meta data.
There was a problem hiding this comment.
The IOException will be cached, and then do the delete operation. So the IOEngine can use after that.
anoopsjohn
reviewed
Sep 16, 2019
| } | ||
| } else { | ||
| // not configure persistent path | ||
| deleteCacheDataFile(); |
There was a problem hiding this comment.
Where we will create the cache files again then?
There was a problem hiding this comment.
The cache files would be created in init() method. rafs[i] = new RandomAccessFile(filePath, "rw"); will create new file if it not exist.
anoopsjohn
reviewed
Sep 16, 2019
| /** | ||
| * Delete bucketcache files | ||
| */ | ||
| void deleteCacheDataFile(); |
There was a problem hiding this comment.
Even this also not needed in Interface at least as of now. If we verify the checksum in BucketCache (as above comment) and then decide to do this delete, ya then it is needed. But in flow when and where we create it again?
|
Hello, @Reidddddd @anoopsjohn , I have created a new pull request. HBASE-22890 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HBASE 22890