HDDS-1981: Datanode should sync db when container is moved to CLOSED or QUASI_CLOSED state #1319
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
|
@supratimdeka Can you please review the PR? |
|
💔 -1 overall
This message was automatically generated. |
supratimdeka
suggested changes
Aug 21, 2019
| // Close container is not expected to be instantaneous. | ||
| compactDB(); | ||
| // We must sync the DB after close operation | ||
| flushAndSyncDB(); |
There was a problem hiding this comment.
updateContainerData takes care to revert the state of the container in case of failure.
Won't that be a problem with flushDB? If flush fails, don't we want the container to remain Open?
Also, related question : what should be the relative order - sync first and then change state to close, both with the writeLock held
Same question applies to quasiClose as well.
There was a problem hiding this comment.
The recent commit moves sync before state change. Any failure in flush should now move to container to UNHEALTHY state in HddsDispatcher#dispatchRequest.
|
|
||
| // It is ok if this operation takes a bit of time. | ||
| // Close container is not expected to be instantaneous. | ||
| compactDB(); |
There was a problem hiding this comment.
question unrelated to this patch : why do we need to run compact during close? what is the benefit?
There was a problem hiding this comment.
Good question. I feel we don't have to run compactDB in quasi close or close.
|
Thanks @lokeshj1703 for working on this. I think running explicit compaction before close might make it heavy operation. Can we run compaction in the background or in a separate thread after closing the container? |
|
💔 -1 overall
This message was automatically generated. |
|
@supratimdeka @bshashikant Thanks for reviewing the PR! I have pushed a commit addressing your comments. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Thanks @lokeshj1703 for updating the patch . The patch looks good to me. I am +1 on this change. |
|
@bshashikant @supratimdeka @nandakumar131 Thanks for reviewing the PR. I have merged it with trunk. |
amahussein
pushed a commit
to amahussein/hadoop
that referenced
this pull request
Oct 29, 2019
…or QUASI_CLOSED state (apache#1319)
RogPodge
pushed a commit
to RogPodge/hadoop
that referenced
this pull request
Mar 25, 2020
…or QUASI_CLOSED state (apache#1319)
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.
Datanode should sync db when container is moved to CLOSED or QUASI_CLOSED state. This will ensure that the metadata of a container is persisted.