-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-10963. Implement a headBlocks API on the Datanode #6774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
errose28
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @xichen01 I just took a quick look at this and have some questions, maybe you could fill in the details.
- What is the overall goal?
I'm not sure I understand the overall feature being implemented here. Is the goal that for a given key in the Ozone manager, you can verify that all the blocks are present without actually reading the data? Please add more details to HDDS-10962 explaining the use and plan for this tool since this will provide context for changes related to it.
- There looks to be overlap with other features
I think there may be some overlap with container reconciliation here (HDDS-10239). In particular that change will require an API to pull the merkle/checksum tree of a container and its blocks. This is eventually consistent since it is computed by the scanner, but it avoids any locking or iteration by simply returning a pre-computed proto. See the abstract/blueprint proposed in the doc and HDDS-10376.
Additionally, ensuring that disk and DB content match is the job of the scanner. This runs on each volume of each datanode and is therefore a much more efficient way to catch and fix consistency issues than a request that a client must make for every container. The scanner also accounts for nuances like container state, block deletion that modifies disk and DB in multiple steps, and optimistic locking. I don't see handling for these in this change and re-implementing them seems like duplicate work. On first glance of this implementation the large iterations done under a read lock also look concerning. The scanner is designed to avoid such things.
- The "head request" terminology is kind of confusing in this context
Head type requests usually return just the metadata about the objects being requested. In this case the proposed API is sort of inverted. If you do "head" of a block, it tells you if the block is not present, instead of telling you it is present based on its metadata.
| try { | ||
| KeyValueContainerData containerData = (KeyValueContainerData) container.getContainerData(); | ||
| if (!containerData.getLayoutVersion().equals(ContainerLayoutVersion.FILE_PER_BLOCK)) { | ||
| throw new UnsupportedEncodingException("Not support Container Layout " + containerData.getLayoutVersion()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the correct type to use. The Java doc says this is for character encodings. We probably want to throw some type of StorageContainerException.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected to UnsupportedOperationException
|
@errose28 Thanks for your detailed response. What is the overall goal?The goal is to eventually implement a tool(command line tool) that can quickly find missing keys. What is the contextThere are two scenarios that require this tool
We can't guarantee that there won't be other reasons for data loss in the future, so we need a tool that can quickly scan the entire cluster and make sure that all keys aren't missing. There looks to be overlap with other features“find missing keys” and "container reconciliation" and "Datanode scanner" have different purposes.
The scanner also accounts for nuances like container state, block deletion that modifies disk and DB in multiple stepsI think “find missing keys” doesn't need to take into account such things as Container state, because find missing key is to find Block replica of keys that are completely missing, and as long as any of these keys “exist” then the key is not missing in the cluster, and the key can be recovered by some way. Does “find missing key” require reading and checking of data?“find missing key” is only used to verify existence, not correctness, which can be guaranteed by the Datanode scanner and checksum. The "head request" terminology is kind of confusing in this contextMakes sense , I think we can change the name of the API. maybe we can rename it to |
|
@xichen01 Based on comments here and in Jira, I think a design doc would be needed, which the community can discuss. https://ozone.apache.org/docs/edge/design/ozone-enhancement-proposals.html |
@adoroszlai @errose28 I have raised a doc about this PR in #7548 |
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
|
Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen it. |
What changes were proposed in this pull request?
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10963
How was this patch tested?
Unit tests.