[HDDS-1200] Add support for checksum verification in data scrubber #1154
Conversation
This file contains hidden or 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
swagle
reviewed
Jul 25, 2019
| public static final boolean HDDS_SCM_SAFEMODE_ENABLED_DEFAULT = true; | ||
| public static final String HDDS_SCM_SAFEMODE_MIN_DATANODE = | ||
| "hdds.scm.safemode.min.datanode"; | ||
| public static final int HDDS_SCM_SAFEMODE_MIN_DATANODE_DEFAULT = 1; | ||
|
|
||
| public static final String HDDS_CONTAINER_SCANNER_VOLUME_BYTES_PER_SECOND = | ||
| "hdds.container.scanner.volume.bytes.per.second"; |
There was a problem hiding this comment.
We could make this a bit more intuitive by adding let's say, throttle.bytes.per.second?
There was a problem hiding this comment.
Sorry my standard Ozone Comment: Can we please use the configuration based API for these changes.
https://cwiki.apache.org/confluence/display/HADOOP/Java-based+configuration+API
There was a problem hiding this comment.
@swagle the property name is loosely modeled after HDFS. So i think we can keep it that way.
@anuengineer thanks for the info. Let me refactor the logic here to use configuration based API.
There was a problem hiding this comment.
Updated the patch to use configuration based APIs.
swagle
reviewed
Jul 25, 2019
| scrub(); | ||
| if (!stopping) { | ||
| try { | ||
| Thread.sleep(300000); /* 5 min between scans */ |
There was a problem hiding this comment.
This should be configurable, no?
There was a problem hiding this comment.
This logic was present in ContainerScrubber.java before this patch. I just refactored it. Let me make it configurable.
anuengineer
reviewed
Jul 25, 2019
| public static final String HDDS_CONTAINER_SCANNER_VOLUME_BYTES_PER_SECOND = | ||
| "hdds.container.scanner.volume.bytes.per.second"; | ||
| public static final long | ||
| HDDS_CONTAINER_SCANNER_VOLUME_BYTES_PER_SECOND_DEFAULT = 1048576L; | ||
|
|
There was a problem hiding this comment.
1048576L , ozone supports writing things like 1MB. if that is useful.
There was a problem hiding this comment.
ok. let me use that.
| } | ||
| BlockUtils.getDB(onDiskContainerData, checkConfig); | ||
| KeyValueBlockIterator kvIter = new KeyValueBlockIterator(containerID, | ||
| new File(onDiskContainerData.getContainerPath()))) { |
There was a problem hiding this comment.
Can you please run this in a profiler mode -- and make sure there are no memory leaks in this code path. Nothing to do with your patch at all. Just that we have found some issues here earlier. Just run it under something like VisualVM and see if we release all memory when get out of the loop.
...service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerCheck.java
Show resolved
Hide resolved
| } catch (IOException ex) { | ||
| long containerId = c.getContainerData().getContainerID(); | ||
| LOG.warn("Unexpected exception while scanning container " | ||
| + containerId, ex); |
There was a problem hiding this comment.
If we are not able to read the container, should we mark the container as unhealthy ? even if we got an exception ? I am not sure if all exceptions do mean contianer is unhealthy, but for some exceptions; yes it is unhealthy.
There was a problem hiding this comment.
Yes we do mark the container as unhealthy in case of I/O errors. But there are some cases where we can not mark a container as unhealthy (e.g. when the rocksdb metadata is deleted or corrupted). In that case we just send an ICR to SCM. Here is relevant code snippet - https://github.com/apache/hadoop/blob/trunk/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java#L900-L915
| * @throws Exception | ||
| */ | ||
| @Test | ||
| public void testKeyValueContainerCheckCorruption() throws Exception { |
There was a problem hiding this comment.
you can run this test under memory profiler to make sure there are no leaks. Thanks
There was a problem hiding this comment.
If you add a label called ozone, then this pull request will be processed by Jenkins. Just FYI. I have done that for this patch.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bshashikant
reviewed
Aug 2, 2019
| sc.getBandwidthPerVolume()), null); | ||
| assertFalse(valid); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a container with normal and deleted blocks. | ||
| * First it will insert normal blocks, and then it will insert |
There was a problem hiding this comment.
Not sure I am following you. Can you elaborate which part do you find misleading? This function was present before this patch ...
bshashikant
reviewed
Aug 2, 2019
| } | ||
| if (v == -1 && i < length) { | ||
| throw new OzoneChecksumException(String | ||
| .format("Inconsistent read for chunk=%s expected length=%d" |
There was a problem hiding this comment.
It might be a good idea to log the blockId as well in the exception msg.
|
Thanks @hgadre for working on this. The patch looks good to me with few minor comments. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
hgadre
force-pushed
the
HDDS-1200
branch
2 times, most recently
from
6a42ca0 to
8d3c73d
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@hgadre Thanks for the contribution. All others thanks for the review. I have committed this patch to the trunk. |
shanthoosh
pushed a commit
to shanthoosh/hadoop
that referenced
this pull request
Oct 15, 2019
…1154) * Fix discrepancy between sql REAL and FLOAT
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.
No description provided.