HBASE-23017 Verify the file integrity in persistent IOEngine #626
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. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ZhaoBQ
force-pushed
the
HBASE-23017
branch
2 times, most recently
from
5ca09dc
to
b797c16
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
anoopsjohn
requested changes
Sep 24, 2019
| @@ -1053,6 +1065,12 @@ private void persistToFile() throws IOException { | |||
| } | |||
| try (FileOutputStream fos = new FileOutputStream(persistencePath, false)) { | |||
| fos.write(ProtobufMagic.PB_MAGIC); | |||
| byte[] checksum = ((PersistentIOEngine) ioEngine).calculateChecksum(algorithm); | |||
There was a problem hiding this comment.
This is not correct.
In master we already have the PB_MAGIC and we are writing the meta info into the persisting file in protobuf way. We can change the PB proto message to include optional checksum also. If the checksum is there, write those bytes. Marking it as optional so retrieving it back as bytes and check whether it was there or not, all these work PB will help us. We dont have to have any extra logic.
| @@ -1081,8 +1101,47 @@ private void retrieveFromFile(int[] bucketSizes) throws IOException { | |||
| throw new IOException("Persistence file does not start with protobuf magic number. " + | |||
| persistencePath); | |||
| } | |||
| byte[] pbuf2 = new byte[pblen]; | |||
There was a problem hiding this comment.
All these logic will go away. From the PB message we can get.. So getting back the checksum should be included as part of parsePB()
There was a problem hiding this comment.
Has been modified to use PB persistent checksum. But I have not designed how to maintain backward compatibility.
|
💔 -1 overall
This message was automatically generated. |
anoopsjohn
requested changes
Sep 27, 2019
| required string map_class = 3; | ||
| map<int32, string> deserializers = 4; | ||
| required BackingMap backing_map = 5; | ||
| optional bytes checksum = 1; |
There was a problem hiding this comment.
Put checksum bytes as the last entry. Do not change the existing order.
| @@ -1067,7 +1081,9 @@ private void retrieveFromFile(int[] bucketSizes) throws IOException { | |||
| } | |||
| assert !cacheEnabled; | |||
|
|
|||
| try (FileInputStream in = deleteFileOnClose(persistenceFile)) { | |||
| * persistent file integrity, default algorithm is MD5 | ||
| * */ | ||
| private String algorithm; | ||
| private byte[] checksum; |
There was a problem hiding this comment.
Why to keep this as state in BC?
| @@ -1053,6 +1066,7 @@ private void persistToFile() throws IOException { | |||
| } | |||
| try (FileOutputStream fos = new FileOutputStream(persistencePath, false)) { | |||
| fos.write(ProtobufMagic.PB_MAGIC); | |||
| checksum = ((PersistentIOEngine) ioEngine).calculateChecksum(algorithm); | |||
| BucketProtoUtils.toPB(this).writeDelimitedTo(fos); | |||
There was a problem hiding this comment.
The checksum can be passed as an argument. Keeping checksum as a state var in BC does not look correct.
| @@ -1131,6 +1155,8 @@ private void verifyCapacityAndClasses(long capacitySize, String ioclass, String | |||
| } | |||
|
|
|||
| private void parsePB(BucketCacheProtos.BucketCacheEntry proto) throws IOException { | |||
| ((PersistentIOEngine) ioEngine).verifyFileIntegrity(proto.getChecksum().toByteArray(), | |||
There was a problem hiding this comment.
For compatibility, u need to check whether the checksum is there or not. Use proto.hasChecksum() before calling the getter.
| @@ -1525,6 +1551,10 @@ float getMemoryFactor() { | |||
| return memoryFactor; | |||
| } | |||
|
|
|||
| public byte[] getChecksum() { | |||
There was a problem hiding this comment.
Is this public getter needed then?
| private static final DuFileCommand DU = new DuFileCommand(new String[] {"du", ""}); | ||
| protected final String[] filePaths; | ||
|
|
||
| public PersistentIOEngine(String[] filePaths) { |
There was a problem hiding this comment.
Why the arg not using String... ? U can just call super(filePath); from FileMmapIOEngine ?
There was a problem hiding this comment.
Because FileIOEngine can use multiple files as cache file.
There was a problem hiding this comment.
I dont mean used String type. Strings only but as in FileIOE.. String... filePaths
If so in FileMmapIOEngine we can just call super(filePath)?
| * Verify cache files's integrity | ||
| * @param algorithm the backingMap persistence path | ||
| */ | ||
| protected void verifyFileIntegrity(byte[] persistentChecksum, String algorithm) |
There was a problem hiding this comment.
Whether this in line with the branch-1 patch now?
There was a problem hiding this comment.
In branch-1 the method verifyFileIntegrity() return boolean type to indicate whether the verification is successful. Here we throw IOE if verification failed. Because the method parsePB() use this way.
There was a problem hiding this comment.
Fine. The IOE is handled in BC.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
anoopsjohn
requested changes
Sep 30, 2019
| algorithm); | ||
| } else { | ||
| // if has not checksum, it means the persistence file is old format | ||
| LOG.warn("Persistence file is old format, it does not support verifying file integrity!"); |
There was a problem hiding this comment.
This needs to be a Warn? It can be just Info right?
| @@ -1235,6 +1253,10 @@ public long getCurrentSize() { | |||
| return this.bucketAllocator.getUsedSize(); | |||
| } | |||
|
|
|||
| public String getAlgorithm() { | |||
| .putAllDeserializers(CacheableDeserializerIdManager.save()) | ||
| .setBackingMap(BucketProtoUtils.toPB(cache.backingMap)) | ||
| .setChecksum(ByteString.copyFrom(((PersistentIOEngine) cache.ioEngine). | ||
| calculateChecksum(cache.getAlgorithm()))).build(); |
There was a problem hiding this comment.
Ok used here. Fine. It could be have been just package protected only not public?
| private static final DuFileCommand DU = new DuFileCommand(new String[] {"du", ""}); | ||
| protected final String[] filePaths; | ||
|
|
||
| public PersistentIOEngine(String[] filePaths) { |
There was a problem hiding this comment.
I dont mean used String type. Strings only but as in FileIOE.. String... filePaths
If so in FileMmapIOEngine we can just call super(filePath)?
| * Verify cache files's integrity | ||
| * @param algorithm the backingMap persistence path | ||
| */ | ||
| protected void verifyFileIntegrity(byte[] persistentChecksum, String algorithm) |
There was a problem hiding this comment.
Fine. The IOE is handled in BC.
|
💔 -1 overall
This message was automatically generated. |
anoopsjohn
approved these changes
Oct 3, 2019
There was a problem hiding this comment.
+1
@Reidddddd Can u also review once and if looks good, commit?
Reidddddd
reviewed
Oct 11, 2019
| algorithm); | ||
| } else { | ||
| // if has not checksum, it means the persistence file is old format | ||
| LOG.info("Persistence file is old format, it does not support verifying file integrity!"); |
There was a problem hiding this comment.
Persistent? Adjective instead of noun?
Reidddddd
reviewed
Oct 11, 2019
| .setMapClass(cache.backingMap.getClass().getName()) | ||
| .putAllDeserializers(CacheableDeserializerIdManager.save()) | ||
| .setBackingMap(BucketProtoUtils.toPB(cache.backingMap)) | ||
| .setChecksum(ByteString.copyFrom(((PersistentIOEngine) cache.ioEngine). |
There was a problem hiding this comment.
Not sure here, since i don't read the code. What if the cache is not PersistentIOEngine? Will here throw cast exception?
There was a problem hiding this comment.
Calling this method has shown that ioEngine is persistent, and all the persistent ioEngine extend PersistentIOEngine. So it won't throw cast exception.
|
💔 -1 overall
This message was automatically generated. |
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-23017