You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/06/14 15:11:08 UTC
[GitHub] [ozone] umamaheswararao opened a new pull request, #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
umamaheswararao opened a new pull request, #3514:
URL: https://github.com/apache/ozone/pull/3514
## What changes were proposed in this pull request?
This patch proposes to write the empty putBlocks on non writing DNs in the case of padding.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6794
## How was this patch tested?
Adding tests.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on pull request #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on PR #3514:
URL: https://github.com/apache/ozone/pull/3514#issuecomment-1168115808
@guihecheng, I have discovered several issues with this. So, I have just kept this issue a side currently as I am focussing on other prioritized tasks.
Here just empty put block will not work:
1. Container may not be available. PutBlock will not create container on absence. Container's usually be created on first write chunk. Since we don't have write chunk, empty putBlocks will just fail.
2. Last putBlock with close flag true will fail even though container is available as chunkFile may not exist. close flag will enforce to flush the content to os file system at the DN.
So, the best way could be to create the container's from client on initializing the streams first.
Here I found a problem that, container's may exist already and exist createContainer API will just fail when container already exist. SO, we may need a createContainerAPI which should not fail when container exist. May be we need additional flag in createContainer API?
With offline recovery, we are creating the container all the time to target nodes first. So, even though no blocks needs to be recovered, we may create the container. So, first recovery task will solve the problem and from then onward we will have empty containers I guess. Only thing we need to make sure is, empty containers should not get delete in EC case from RM. With this, I felt this can be low prioritized, until RM work is done.
Doing it as part of close or start of input stream should not have any difference. In both cases, createContainer will fail if already exist.
Please not current code is just experimental changes. Not for commit or review. Feel free to suggest or take up if you have any other thoughts which may be simpler and low risk.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on pull request #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on PR #3514:
URL: https://github.com/apache/ozone/pull/3514#issuecomment-1177803164
Thanks a lot @JacksonYao287 for the reviews!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a diff in pull request #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on code in PR #3514:
URL: https://github.com/apache/ozone/pull/3514#discussion_r915787932
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -229,10 +229,17 @@ protected List<ChunkInfo> getChunkInfos() throws IOException {
blockID.getContainerID());
}
- DatanodeBlockID datanodeBlockID = blockID
- .getDatanodeBlockIDProtobuf();
+ DatanodeBlockID.Builder blkIDBuilder =
+ DatanodeBlockID.newBuilder().setContainerID(blockID.getContainerID())
+ .setLocalID(blockID.getLocalID())
+ .setBlockCommitSequenceId(blockID.getBlockCommitSequenceId());
Review Comment:
blockCommitSequenceID is used for handling quosi_closed ratis container which has a non-determined state. so i don`t think it has any use for EC container. maybe we can remove it for EC case?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a diff in pull request #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3514:
URL: https://github.com/apache/ozone/pull/3514#discussion_r916014835
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -229,10 +229,17 @@ protected List<ChunkInfo> getChunkInfos() throws IOException {
blockID.getContainerID());
}
- DatanodeBlockID datanodeBlockID = blockID
- .getDatanodeBlockIDProtobuf();
+ DatanodeBlockID.Builder blkIDBuilder =
+ DatanodeBlockID.newBuilder().setContainerID(blockID.getContainerID())
+ .setLocalID(blockID.getLocalID())
+ .setBlockCommitSequenceId(blockID.getBlockCommitSequenceId());
Review Comment:
We have not really thought much about BCSID. Currently we are not using it, but we need to think whether we have benefit or take advantage of any other scenarios which we have not covered yet. So, I think it may be too early to discard. Eventually we can ignore that later times if we never find any use of it. I suggest to get set as it is available now. Also we surely don't want to ignore in BlockInputStream as that is non EC code flow too.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on pull request #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3514:
URL: https://github.com/apache/ozone/pull/3514#issuecomment-1172576454
Thanks for working on this @umamaheswararao. Actually the normal EC write path doesn't create blocks on non-writing node, that's why `TestContainerCommandsEC.testListBlock` failed. You can remove the failed assertion if this behavior needs to be changed.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on pull request #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on PR #3514:
URL: https://github.com/apache/ozone/pull/3514#issuecomment-1177807875
Since I got a +1, I am going ahead to commit this. @nandakumar131 @sodonnel @guihecheng @adoroszlai Let me know if you have concerns on this approach or anything we missed to take care with respective to SCM HA etc. Thanks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on pull request #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3514:
URL: https://github.com/apache/ozone/pull/3514#issuecomment-1172599226
> I should say that RM won't be able to trigger Offline Recovery facing a CLOSED tiny container with possibly only one partial stripe since it doesn't have enough container replica reports gathered. So we need to handle this problem specially on the write path or somewhere else. The problem is likely to hit when doing Offline Recovery test with small files(e.g. 1MB) only, it may not be very easy to reveal on real mixed workloads, but still possible.
This is indeed a problem. I would suggest to allow empty EC containers, and generalize the problem.
(Manage EC container in groups, not as individual)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao merged pull request #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
Posted by GitBox <gi...@apache.org>.
umamaheswararao merged PR #3514:
URL: https://github.com/apache/ozone/pull/3514
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] guihecheng commented on pull request #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on PR #3514:
URL: https://github.com/apache/ozone/pull/3514#issuecomment-1164160380
Hi @umamaheswararao any updates on this one?
And I have an idea: could we create the empty container entity on DN on the CLOSE of EC pipelines?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] guihecheng commented on pull request #3514: HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on PR #3514:
URL: https://github.com/apache/ozone/pull/3514#issuecomment-1168143820
> @guihecheng, I have discovered several issues with this. So, I have just kept this issue a side currently as I am focussing on other prioritized tasks.
>
> Here just empty put block will not work:
>
> 1. Container may not be available. PutBlock will not create container on absence. Container's usually be created on first write chunk. Since we don't have write chunk, empty putBlocks will just fail.
> 2. Last putBlock with close flag true will fail even though container is available as chunkFile may not exist. close flag will enforce to flush the content to os file system at the DN.
>
> So, the best way could be to create the container's from client on initializing the streams first. Here I found a problem that, container's may exist already and exist createContainer API will just fail when container already exist. SO, we may need a createContainerAPI which should not fail when container exist. May be we need additional flag in createContainer API?
>
> With offline recovery, we are creating the container all the time to target nodes first. So, even though no blocks needs to be recovered, we may create the container. So, first recovery task will solve the problem and from then onward we will have empty containers I guess. Only thing we need to make sure is, empty containers should not get delete in EC case from RM. With this, I felt this can be low prioritized, until RM work is done.
>
> Doing it as part of close or start of input stream should not have any difference. In both cases, createContainer will fail if already exist.
>
> Please not current code is just experimental changes. Not for commit or review. Feel free to suggest or take up if you have any other thoughts which may be simpler and low risk.
Thanks for the info, it is really a problem that needs to be discuss more about, since the EC on-disk container replica is different from the Ratis-Replicated container replica, then some old assumptions(e.g. create container replica on the first WriteChunk) may not apply.
I should say that RM won't be able to trigger Offline Recovery facing a CLOSED tiny container with possibly only one partial stripe since it doesn't have enough container replica reports gathered. So we need to handle this problem specially on the write path or somewhere else. The problem is likely to hit when doing Offline Recovery test with small files(e.g. 1MB) only, it may not be very easy to reveal on real mixed workloads, but still possible.
Create container replicas on init of BlockStreams is a workable idea as I think, but it may possibly bring a performance impact. I'm thinking that maybe we could handle this during the OPEN/CLOSE of the container/pipeline on SCM side, but it also has problems that we don't have synchronous view of container replica creation, and we can't wait for the creations.
Let's dig more about this later.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org