Historical unloads damaged segments automatically when lazy on start.#10688
Merged
clintropolis merged 34 commits intoapache:masterfrom Jan 17, 2021
Merged
Conversation
Contributor
|
How about trigger unload or force delete when the query failed due to segment corruption? After the unload, the coordinator will ask another historical to load this segment. |
Contributor
Author
|
Hi @kaijianding. As you can see, there are few error messages when query failed due to segments damaged. So that it is hard to distinguish what kind of exception is caused by damaged segments, wrong query or others and find out which specific files are damaged. Because a query usually reaches multiple segments. What's more, let query failure trigger a |
Contributor
|
@zhangyue19921010 I guess the exception from loadIndex can be a signal of corruption. When you distinguish the corruption exception, it is safe to unload this segment and let the coordinator choose another historical to load(usually, you have replicas). Compare to restart behavior, I think the unloading behavior is more efficient and no human involvement is needed. |
Contributor
Author
|
Hi @kaijianding. This PR is just aiming to let druid regain the ability which is broken by lazyOnStart to check the availability of all segments and reload corrupted segments during historical restart. Is what you proposed that make druid have the ability to find and solve corrupted segments during runtime no matter lazyOnStart or not? It is actually a nice idea which can enhance the robustness of Druid and solve the corrupted segments problems once and for all. I will keep digging maybe make an another PR to make it happen if I understand right. What do you think? :) |
Contributor
Author
Unfortunately there is no extra exception in |
Contributor
Only when lazy load should be good enough. Maybe do something in this IOException catch and trigger SegmentManager.dropSegment(dataSegment) somehow. |
added 3 commits
January 5, 2021 11:45
zhangyue19921010
force-pushed
the
historical-lazyOnStart-with-fileCheck
branch
from
46303d4 to
7775658
Compare
Contributor
Author
|
Hi @kaijianding , sorry for late response. I just change the strategy for lazyOnStart action as you suggested. |
Contributor
Author
|
Here is the full logs during query failed and trigger unload action. damage segment files -> lazy On start -> query failed -> segment unload automatically -> load segment according to coordinator -> query successfully. |
kaijianding
requested changes
Jan 6, 2021
|
|
||
| package org.apache.druid.coordination; | ||
|
|
||
| public interface CommonCallback |
Contributor
There was a problem hiding this comment.
Use a more meaningful name, like SegmentLazyLoadFailCallback
| { | ||
| return loadIndex(inDir, false); | ||
| return loadIndex(inDir, false, CommonCallback.NOOP); | ||
| } |
|
|
||
| private ColumnHolder deserializeColumn(ObjectMapper mapper, ByteBuffer byteBuffer, SmooshedFileMapper smooshedFiles) | ||
| throws IOException | ||
| throws IOException, RuntimeException |
Contributor
There was a problem hiding this comment.
why RuntimeException here?
Contributor
Author
There was a problem hiding this comment.
No need actually. Remove now.
kaijianding
reviewed
Jan 6, 2021
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.druid.coordination; |
Contributor
There was a problem hiding this comment.
I guess this interface should be in org.apache.druid.segment, beside the IndexIO
clintropolis
reviewed
Jan 6, 2021
|
|
||
| package org.apache.druid.coordination; | ||
|
|
||
| public interface CommonCallback |
Member
There was a problem hiding this comment.
why not just use Runnable? There is even a Runnables.getNoopRunnable()
Contributor
There was a problem hiding this comment.
I think a meaningful name is better for code readiness through the method arguments
Member
There was a problem hiding this comment.
I don't feel super strongly about this, while I find it pretty obvious what Runnable does and don't think a new interface adds much here, I can see your point as well.
I would agree that if we insist on using a separate interface that it should have a better name more indicative of its function.
Contributor
Author
There was a problem hiding this comment.
Change the name to SegmentLazyLoadFailCallback as you guys suggested. Thanks for review :)
kaijianding
reviewed
Jan 6, 2021
| * @throws SegmentLoadingException if the segment cannot be loaded | ||
| */ | ||
| public boolean loadSegment(final DataSegment segment, boolean lazy) throws SegmentLoadingException | ||
| public boolean loadSegment(final DataSegment segment, boolean lazy, CommonCallback loadFailed) throws SegmentLoadingException |
Contributor
There was a problem hiding this comment.
add a new @param in the method comment
Contributor
|
And rename the PR title to match the changes |
kaijianding
approved these changes
Jan 14, 2021
| # resolving the SIGAR dependency. | ||
| - > | ||
| MAVEN_OPTS='-Xmx800m' ${MVN} test -pl ${MAVEN_PROJECTS} | ||
| MAVEN_OPTS='-Xmx1100m' ${MVN} test -pl ${MAVEN_PROJECTS} |
Contributor
Author
There was a problem hiding this comment.
Current UTs don't cover load segment in a lazy way. This PR add a UT to test lazy loading which needs extra memory.
Member
There was a problem hiding this comment.
👍 was just surprised how much more it needed, if travis passes then I assume the change is fine, was just curious mostly
Member
|
I know some of our travis integration tests have been a bit flakey lately, but could you avoid doing this to trigger CI: |
Contributor
Author
Sure, Sorry for that. I don't have the permission for re-running a single job so I have to re-run the whole CI :( |
clintropolis
changed the title
Contributor
Author
|
@kaijianding @clintropolis Thanks for your review and merge! |
9 tasks
JulianJaffePinterest
pushed a commit
to JulianJaffePinterest/druid
that referenced
this pull request
Jan 22, 2021
…apache#10688) * ready to test * tested on dev cluster * tested * code review * add UTs * add UTs * ut passed * ut passed * opti imports * done * done * fix checkstyle * modify uts * modify logs * changing the package of SegmentLazyLoadFailCallback.java to org.apache.druid.segment * merge from master * modify import orders * merge from master * merge from master * modify logs * modify docs * modify logs to rerun ci * modify logs to rerun ci * modify logs to rerun ci * modify logs to rerun ci * modify logs to rerun ci * modify logs to rerun ci * modify logs to rerun ci * modify logs to rerun ci Co-authored-by: yuezhang <yuezhang@freewheel.tv>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Description
The feature mentioned in #6988 is very useful and can greatly improve the speed of historical node restart. But there are some potential risks due to push the loading work to query time.
If segment files like
meta.smooshordata.smooshwhich Historical is served already are damaged, Historical with lazyOnStart enable can restart successfully but will not detect file corruption during restarting. When queries hit these damaged files, these queries will fail and will not expose which specific files are damaged like mentioned in #10608Tests
Here are the undamaged segment files.
First case : meta.smoosh is damaged, for example
Remove a line
video_cro_network_name,0,997718245,1000412345in that meta.smoosh file.Restart Historical node with lazy on start enable.
Second case : data.smoosh is damaged, for example
Delete several bytes in that 00000.smoosh file randomly.
Restart Historical node with lazy on start enable.
In all scenarios, Historical can restart successfully, but queries will failed.
Solution
This PR make Historical with lazy on start have the ability that:
Failed queries caused by damaged segment files will trigger unload action automatically. After the unload, the coordinator will ask another historical to load this segment.
Through this PR, the system heals itself and the administrator need to do nothing.
This PR has:
Key changed/added classes in this PR
IndexIO.java