[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r324975888
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
##
@@ -1021,41 +1033,48 @@ void doDrain(final List entries) throws
InterruptedException {
private void persistToFile() throws IOException {
assert !cacheEnabled;
-FileOutputStream fos = null;
-ObjectOutputStream oos = null;
-try {
+try (ObjectOutputStream oos = new ObjectOutputStream(
+ new FileOutputStream(persistencePath, false))){
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) {
+oos.write(ProtobufUtil.PB_MAGIC);
+byte[] checksum = ((PersistentIOEngine) ioEngine).calculateChecksum();
+oos.writeInt(checksum.length);
+oos.write(checksum);
+ }
oos.writeLong(cacheCapacity);
oos.writeUTF(ioEngine.getClass().getName());
oos.writeUTF(backingMap.getClass().getName());
oos.writeObject(deserialiserMap);
oos.writeObject(backingMap);
-} finally {
- if (oos != null) oos.close();
- if (fos != null) fos.close();
+} catch (NoSuchAlgorithmException e) {
+ LOG.error("No such algorithm : " + algorithm + "! Failed to persist data
on exit",e);
}
}
@SuppressWarnings("unchecked")
- private void retrieveFromFile(int[] bucketSizes) throws IOException,
BucketAllocatorException,
+ private void retrieveFromFile(int[] bucketSizes) throws IOException,
ClassNotFoundException {
File persistenceFile = new File(persistencePath);
if (!persistenceFile.exists()) {
return;
}
assert !cacheEnabled;
-FileInputStream fis = null;
-ObjectInputStream ois = null;
-try {
+try (ObjectInputStream ois = new ObjectInputStream(new
FileInputStream(persistencePath))){
if (!ioEngine.isPersistent())
throw new IOException(
"Attempt to restore non-persistent cache mappings!");
- fis = new FileInputStream(persistencePath);
- ois = new ObjectInputStream(fis);
+ // for backward compatibility
+ if (ioEngine instanceof PersistentIOEngine &&
+!((PersistentIOEngine) ioEngine).isOldVersion()) {
+byte[] PBMagic = new byte[ProtobufUtil.PB_MAGIC.length];
+ois.read(PBMagic);
+int length = ois.readInt();
+byte[] persistenceChecksum = new byte[length];
+ois.read(persistenceChecksum);
Review 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.
@Reidd
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode URL: https://github.com/apache/hbase/pull/528#discussion_r324590529 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java ## @@ -1078,9 +1097,8 @@ private void retrieveFromFile(int[] bucketSizes) throws IOException, BucketAlloc bucketAllocator = allocator; deserialiserMap = deserMap; backingMap = backingMapFromFile; + blockNumber.set(backingMap.size()); Review comment: I see. So this is an existing bug. Its a one liner change. Still can be done as another bug jira may be. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r324575040
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/PersistentIOEngine.java
##
@@ -0,0 +1,59 @@
+/**
+ * Copyright The Apache Software Foundation
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.hadoop.hbase.io.hfile.bucket;
+
+import java.io.IOException;
+import java.security.NoSuchAlgorithmException;
+
+import org.apache.hadoop.hbase.classification.InterfaceAudience;
+
+/**
+ * A class implementing PersistentIOEngine interface supports persistent and
file integrity verify
+ * for {@link BucketCache}
+ */
+@InterfaceAudience.Private
+public interface PersistentIOEngine extends IOEngine {
+
+ /**
+ * Delete bucketcache files
+ */
+ void deleteCacheDataFile();
Review 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?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r324573895
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/FileIOEngine.java
##
@@ -19,46 +19,96 @@
package org.apache.hadoop.hbase.io.hfile.bucket;
import java.io.File;
+import java.io.FileInputStream;
import java.io.IOException;
+import java.io.ObjectInputStream;
import java.io.RandomAccessFile;
import java.nio.ByteBuffer;
import java.nio.channels.ClosedByInterruptException;
import java.nio.channels.ClosedChannelException;
import java.nio.channels.FileChannel;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
import java.util.Arrays;
import java.util.concurrent.locks.ReentrantLock;
import com.google.common.annotations.VisibleForTesting;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.hadoop.hbase.classification.InterfaceAudience;
+import org.apache.hadoop.hbase.protobuf.ProtobufUtil;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.util.Shell;
import org.apache.hadoop.util.StringUtils;
/**
* IO engine that stores data to a file on the local file system.
*/
@InterfaceAudience.Private
-public class FileIOEngine implements IOEngine {
+public class FileIOEngine implements PersistentIOEngine {
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", ""});
+
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 oldVersion;
private FileReadAccessor readAccessor = new FileReadAccessor();
private FileWriteAccessor writeAccessor = new FileWriteAccessor();
- public FileIOEngine(long capacity, String... filePaths) throws IOException {
+ public FileIOEngine(String algorithmName, String persistentPath,
+long capacity, String... filePaths) throws IOException {
this.sizePerFile = capacity / filePaths.length;
this.capacity = this.sizePerFile * filePaths.length;
this.filePaths = filePaths;
this.fileChannels = new FileChannel[filePaths.length];
this.rafs = new RandomAccessFile[filePaths.length];
this.channelLocks = new ReentrantLock[filePaths.length];
+this.algorithmName = algorithmName;
+verifyFileIntegrity(persistentPath);
+init();
+ }
+
+ /**
+ * Verify cache files's integrity
+ * @param persistentPath the backingMap persistent path
+ */
+ @Override
+ public void verifyFileIntegrity(String persistentPath) {
+if (persistentPath != null) {
+ byte[] persistentChecksum = readPersistentChecksum(persistentPath);
+ if (!oldVersion) {
+try {
+ byte[] calculateChecksum = calculateChecksum();
+ 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();
+ }
+} catch (IOException ioex) {
+ LOG.error("File verification failed because of ", ioex);
+ // delete cache files and backingMap persistent file.
+ deleteCacheDataFile();
+ new File(persistentPath).delete();
+} catch (NoSuchAlgorithmException nsae) {
+ LOG.error("No such algorithm " + algorithmName, nsae);
+ throw new RuntimeException(nsae);
+}
+ }
+} else {
+ // not configure persistent path
+ deleteCacheDataFile();
Review comment:
Where we will create the cache files again then?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r324573285
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/FileIOEngine.java
##
@@ -19,46 +19,96 @@
package org.apache.hadoop.hbase.io.hfile.bucket;
import java.io.File;
+import java.io.FileInputStream;
import java.io.IOException;
+import java.io.ObjectInputStream;
import java.io.RandomAccessFile;
import java.nio.ByteBuffer;
import java.nio.channels.ClosedByInterruptException;
import java.nio.channels.ClosedChannelException;
import java.nio.channels.FileChannel;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
import java.util.Arrays;
import java.util.concurrent.locks.ReentrantLock;
import com.google.common.annotations.VisibleForTesting;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.hadoop.hbase.classification.InterfaceAudience;
+import org.apache.hadoop.hbase.protobuf.ProtobufUtil;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.util.Shell;
import org.apache.hadoop.util.StringUtils;
/**
* IO engine that stores data to a file on the local file system.
*/
@InterfaceAudience.Private
-public class FileIOEngine implements IOEngine {
+public class FileIOEngine implements PersistentIOEngine {
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", ""});
+
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 oldVersion;
private FileReadAccessor readAccessor = new FileReadAccessor();
private FileWriteAccessor writeAccessor = new FileWriteAccessor();
- public FileIOEngine(long capacity, String... filePaths) throws IOException {
+ public FileIOEngine(String algorithmName, String persistentPath,
+long capacity, String... filePaths) throws IOException {
this.sizePerFile = capacity / filePaths.length;
this.capacity = this.sizePerFile * filePaths.length;
this.filePaths = filePaths;
this.fileChannels = new FileChannel[filePaths.length];
this.rafs = new RandomAccessFile[filePaths.length];
this.channelLocks = new ReentrantLock[filePaths.length];
+this.algorithmName = algorithmName;
+verifyFileIntegrity(persistentPath);
+init();
+ }
+
+ /**
+ * Verify cache files's integrity
+ * @param persistentPath the backingMap persistent path
+ */
+ @Override
+ public void verifyFileIntegrity(String persistentPath) {
+if (persistentPath != null) {
+ byte[] persistentChecksum = readPersistentChecksum(persistentPath);
+ if (!oldVersion) {
+try {
+ byte[] calculateChecksum = calculateChecksum();
+ 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();
Review 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.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r324572000
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
##
@@ -1021,41 +1033,48 @@ void doDrain(final List entries) throws
InterruptedException {
private void persistToFile() throws IOException {
assert !cacheEnabled;
-FileOutputStream fos = null;
-ObjectOutputStream oos = null;
-try {
+try (ObjectOutputStream oos = new ObjectOutputStream(
+ new FileOutputStream(persistencePath, false))){
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) {
+oos.write(ProtobufUtil.PB_MAGIC);
+byte[] checksum = ((PersistentIOEngine) ioEngine).calculateChecksum();
+oos.writeInt(checksum.length);
+oos.write(checksum);
+ }
oos.writeLong(cacheCapacity);
oos.writeUTF(ioEngine.getClass().getName());
oos.writeUTF(backingMap.getClass().getName());
oos.writeObject(deserialiserMap);
oos.writeObject(backingMap);
-} finally {
- if (oos != null) oos.close();
- if (fos != null) fos.close();
+} catch (NoSuchAlgorithmException e) {
+ LOG.error("No such algorithm : " + algorithm + "! Failed to persist data
on exit",e);
}
}
@SuppressWarnings("unchecked")
- private void retrieveFromFile(int[] bucketSizes) throws IOException,
BucketAllocatorException,
+ private void retrieveFromFile(int[] bucketSizes) throws IOException,
ClassNotFoundException {
File persistenceFile = new File(persistencePath);
if (!persistenceFile.exists()) {
return;
}
assert !cacheEnabled;
-FileInputStream fis = null;
-ObjectInputStream ois = null;
-try {
+try (ObjectInputStream ois = new ObjectInputStream(new
FileInputStream(persistencePath))){
if (!ioEngine.isPersistent())
throw new IOException(
"Attempt to restore non-persistent cache mappings!");
- fis = new FileInputStream(persistencePath);
- ois = new ObjectInputStream(fis);
+ // for backward compatibility
+ if (ioEngine instanceof PersistentIOEngine &&
+!((PersistentIOEngine) ioEngine).isOldVersion()) {
+byte[] PBMagic = new byte[ProtobufUtil.PB_MAGIC.length];
+ois.read(PBMagic);
+int length = ois.readInt();
+byte[] persistenceChecksum = new byte[length];
+ois.read(persistenceChecksum);
Review 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.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode URL: https://github.com/apache/hbase/pull/528#discussion_r324567199 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java ## @@ -1078,9 +1097,8 @@ private void retrieveFromFile(int[] bucketSizes) throws IOException, BucketAlloc bucketAllocator = allocator; deserialiserMap = deserMap; backingMap = backingMapFromFile; + blockNumber.set(backingMap.size()); Review comment: Why this change? Is it related to this jira directly? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r324566217
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
##
@@ -1021,41 +1033,48 @@ void doDrain(final List entries) throws
InterruptedException {
private void persistToFile() throws IOException {
assert !cacheEnabled;
-FileOutputStream fos = null;
-ObjectOutputStream oos = null;
-try {
+try (ObjectOutputStream oos = new ObjectOutputStream(
+ new FileOutputStream(persistencePath, false))){
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) {
+oos.write(ProtobufUtil.PB_MAGIC);
+byte[] checksum = ((PersistentIOEngine) ioEngine).calculateChecksum();
+oos.writeInt(checksum.length);
+oos.write(checksum);
+ }
oos.writeLong(cacheCapacity);
oos.writeUTF(ioEngine.getClass().getName());
oos.writeUTF(backingMap.getClass().getName());
oos.writeObject(deserialiserMap);
oos.writeObject(backingMap);
-} finally {
- if (oos != null) oos.close();
- if (fos != null) fos.close();
+} catch (NoSuchAlgorithmException e) {
+ LOG.error("No such algorithm : " + algorithm + "! Failed to persist data
on exit",e);
}
}
@SuppressWarnings("unchecked")
- private void retrieveFromFile(int[] bucketSizes) throws IOException,
BucketAllocatorException,
+ private void retrieveFromFile(int[] bucketSizes) throws IOException,
ClassNotFoundException {
File persistenceFile = new File(persistencePath);
if (!persistenceFile.exists()) {
return;
}
assert !cacheEnabled;
-FileInputStream fis = null;
-ObjectInputStream ois = null;
-try {
+try (ObjectInputStream ois = new ObjectInputStream(new
FileInputStream(persistencePath))){
if (!ioEngine.isPersistent())
throw new IOException(
"Attempt to restore non-persistent cache mappings!");
- fis = new FileInputStream(persistencePath);
- ois = new ObjectInputStream(fis);
+ // for backward compatibility
+ if (ioEngine instanceof PersistentIOEngine &&
Review comment:
Same as above comment. See above
if (!ioEngine.isPersistent()) throw new IOException().
Just after that line itself you can do the typecast.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r324565575
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
##
@@ -1021,41 +1033,48 @@ void doDrain(final List entries) throws
InterruptedException {
private void persistToFile() throws IOException {
assert !cacheEnabled;
-FileOutputStream fos = null;
-ObjectOutputStream oos = null;
-try {
+try (ObjectOutputStream oos = new ObjectOutputStream(
+ new FileOutputStream(persistencePath, false))){
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) {
Review 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.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r321664376
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/PersistentIOEngineUtils.java
##
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.hfile.bucket;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.classification.InterfaceAudience;
+import org.apache.hadoop.hbase.protobuf.ProtobufUtil;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.util.Shell;
+
+/**
+ * Utilities for FileIOEngine's file operation
+ */
+@InterfaceAudience.Private
+public final class PersistentIOEngineUtils {
+ private static final Log LOG =
LogFactory.getLog(PersistentIOEngineUtils.class);
+ private static final DuFileCommand du = new DuFileCommand(new String[]
{"du", ""});
+
+ private PersistentIOEngineUtils() {
+ }
+
+ /**
+ * Read the persistence checksum from persistence path
+ * @param persistencePath the backingMap persistence path
+ * @return the persistence checksum
+ */
+ public static byte[] readPersistenceChecksum(String persistencePath) {
+if (persistencePath == null) {
+ return null;
+}
+try (ObjectInputStream ois = new ObjectInputStream(new
FileInputStream(persistencePath))) {
+ byte[] PBMagic = new byte[ProtobufUtil.PB_MAGIC.length];
+ ois.read(PBMagic);
+ if (Bytes.equals(ProtobufUtil.PB_MAGIC, PBMagic)) {
+int length = ois.readInt();
+byte[] persistenceChecksum = new byte[length];
+ois.read(persistenceChecksum);
+return persistenceChecksum;
+ }
+} catch (IOException ioex) {
+ LOG.warn("Failed read persistent checksum, because of " + ioex);
+ return null;
+}
+return null;
+ }
+
+ /**
+ * Delete bucketcache files
+ * @param filePaths the cache data files
+ */
+ public static void deleteCacheDataFile(String[] filePaths) {
+if (filePaths == null) {
+ return;
+}
+for (String file : filePaths) {
+ new File(file).delete();
+}
+ }
+
+ /**
+ * Using an encryption algorithm to get a checksum, the default encryption
algorithm is MD5
+ * @param ioEngineName the IOEngine name
+ * @param algorithmName the encryption algorithm
+ * @return the checksum which is convert to HexString
+ * @throws IOException something happened like file not exists
+ * @throws NoSuchAlgorithmException no such algorithm
+ */
+ public static byte[] calculateChecksum(String ioEngineName, String
algorithmName)
+throws IOException, NoSuchAlgorithmException {
+String[] filePaths =
+ ioEngineName.substring(ioEngineName.indexOf(":") +
1).split(FileIOEngine.FILE_DELIMITER);
+return calculateChecksum(filePaths, algorithmName);
+ }
+
+ public static byte[] calculateChecksum(String[] filePaths, String
algorithmName)
+throws IOException, NoSuchAlgorithmException {
+if (filePaths == null) {
+ return null;
+}
+StringBuilder sb = new StringBuilder();
+for (String filePath : filePaths){
+ File file = new File(filePath);
+ if (file.exists()){
+sb.append(getFileSize(filePath));
Review comment:
Should we include the file path also here for checksum calc? We dont
persist the initial cache file paths anywhere. Have size and lastTS which is
good. Just an ask.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r321666510
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
##
@@ -1021,41 +1033,45 @@ void doDrain(final List entries) throws
InterruptedException {
private void persistToFile() throws IOException {
assert !cacheEnabled;
-FileOutputStream fos = null;
-ObjectOutputStream oos = null;
-try {
+try (ObjectOutputStream oos = new ObjectOutputStream(
+ new FileOutputStream(persistencePath, false))){
if (!ioEngine.isPersistent()) {
throw new IOException("Attempt to persist non-persistent cache
mappings!");
}
- fos = new FileOutputStream(persistencePath, false);
- oos = new ObjectOutputStream(fos);
+ oos.write(ProtobufUtil.PB_MAGIC);
+ byte[] checksum =
PersistentIOEngineUtils.calculateChecksum(ioEngineName, algorithm);
Review comment:
I get ur view on placing this checksum info.. The persist really comes into
BucketCache as it persists meta info like backingMap. Here the adjustment is
keeping the ioEngineName as instance variable and the file paths are again
derived from it.In case of verify step that is very evident. The readback
from persistent file has to happen in BucketCache. And then the verification
should be in FileIOE.
How abt doing it this way?
We have isPersistent() in IOEngine. Can we have an extended interface
PersistentIOEngine which will have methods to get the checksum to persist and
also for verify? Within BC we can call these methods at appropriate places.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r321633344
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/PersistentIOEngineUtils.java
##
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.hfile.bucket;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.protobuf.ProtobufUtil;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.util.Shell;
+
+/**
+ * Utilities for FileIOEngine's file operation
+ */
+public final class PersistentIOEngineUtils {
Review comment:
Need Private annotation.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode URL: https://github.com/apache/hbase/pull/528#discussion_r321631322 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/IOEngine.java ## @@ -62,4 +62,9 @@ * Shutdown the IOEngine */ void shutdown(); + + /** + * @return the file paths if the cache is persisted in file + */ + String[] getFilePaths(); Review comment: Seems we no longer need this. Correct? Pls remove 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r320564397
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
##
@@ -242,6 +245,17 @@ public int compare(BlockCacheKey a, BlockCacheKey b) {
/** In-memory bucket size */
private float memoryFactor;
+ private String[] filePaths;
Review comment:
getFilePaths() in IOEngine? That seems strange. The IOEngine need not be
file backed always right? Can we have a way in IOEngine to do this checksum
calc and write as well as verification within the specific IOE impl class?
(Rather than in BucketCache). We have FileMmapIOE also which also needs this
right?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the files when RegionServer is starting and BucketCache is in file mode
anoopsjohn commented on a change in pull request #528: HBASE-22890 Verify the
files when RegionServer is starting and BucketCache is in file mode
URL: https://github.com/apache/hbase/pull/528#discussion_r320359865
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
##
@@ -242,6 +245,17 @@ public int compare(BlockCacheKey a, BlockCacheKey b) {
/** In-memory bucket size */
private float memoryFactor;
+ private String[] filePaths;
Review comment:
filePaths applicable for file based BC only. Now this is made as a member
variable in BucketCache itself. Showing a code level bad smell? At the
IOEngine level we have isPersistent() API. Same way we should have API in
IOEngine for generating and verifying the checksum?
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:
us...@infra.apache.org
With regards,
Apache Git Services
