Skip to content

HDDS-5483. Validate block file length during put block#2463

Merged
mukul1987 merged 2 commits intoapache:HDDS-4454from
sadanand48:HDDS-5483
Aug 2, 2021
Merged

HDDS-5483. Validate block file length during put block#2463
mukul1987 merged 2 commits intoapache:HDDS-4454from
sadanand48:HDDS-5483

Conversation

@sadanand48
Copy link
Contributor

What changes were proposed in this pull request?

Validate block file length during put block .

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-5483

How was this patch tested?

Only a validation check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what conditions could this be violated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smengcl This is just a safety check. The ozone streaming write path will not use the current ratisAsyncApi which ensures sequential transactions i.e putBlock would always occur after writeChunk. To be on a safer side, this check is added. We may remove this after the complete streaming implementation if this check is redundant
cc : @mukul1987

Copy link
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@smengcl smengcl changed the title HDDS-5483.Validate block file length during put block HDDS-5483. Validate block file length during put block Jul 28, 2021
@sadanand48 sadanand48 force-pushed the HDDS-5483 branch 3 times, most recently from 37ad163 to e3f9824 Compare July 28, 2021 15:08
@ferhui
Copy link
Contributor

ferhui commented Aug 2, 2021

Maybe this should rebase on HDDS-4454

@sadanand48 sadanand48 force-pushed the HDDS-5483 branch 2 times, most recently from 28191df to b78221e Compare August 2, 2021 05:41
@sadanand48 sadanand48 closed this Aug 2, 2021
@sadanand48 sadanand48 reopened this Aug 2, 2021
Copy link
Contributor

@mukul1987 mukul1987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM

@mukul1987 mukul1987 merged commit 67c3771 into apache:HDDS-4454 Aug 2, 2021
@szetszwo
Copy link
Contributor

szetszwo commented Aug 12, 2021

@sadanand48 , @mukul1987 , Should this be merged to the master branch instead the HDDS-4454 branch? This dose not look like specific to HDDS-4454.

@szetszwo
Copy link
Contributor

@sadanand48 , @mukul1987 if there is no objection, I will merge this to the master branch. Thanks.

@mukul1987
Copy link
Contributor

@szetszwo, sure lets do that

@szetszwo
Copy link
Contributor

Thanks for the response. Just have merged this to the master branch.

Comment on lines +468 to +471
if (layoutVersion == FILE_PER_BLOCK &&
// ChunkManagerDummyImpl doesn't persist to disk, don't check here
!chunkManager.getClass()
.isAssignableFrom(ChunkManagerDummyImpl.class)) {
Copy link
Contributor

@adoroszlai adoroszlai Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think layout-specific code would be better added to each ChunkManager implementation instead of such == and isAssignableFrom checks. Take a look at finishWriteChunks() and shutdown().

@adoroszlai
Copy link
Contributor

Should this be merged to the master branch instead the HDDS-4454 branch?

Although I don't expect any problems for this specific change, I would prefer going via PR for master next time, to make sure all checks pass.

@szetszwo
Copy link
Contributor

@adoroszlai , it is a good idea to have a PR. I will revert the commit from the master.

szetszwo added a commit that referenced this pull request Aug 19, 2021
@szetszwo
Copy link
Contributor

commit a595d06450d6788237bd459a1ac4f36fe5582992 (HEAD -> master, origin/master, origin/HEAD)
Author: Tsz-Wo Nicholas Sze <szetszwo@apache.org>
Date:   Thu Aug 19 10:53:26 2021 +0800

    Revert "HDDS-5483. Validate block file length during put block (#2463)"
    
    This reverts commit 375f3c4f02a46e12fcfc1acc127ecdc5badd359d.

@szetszwo
Copy link
Contributor

@sadanand48 , as suggested by @adoroszlai , please submit a PR for the master branch. Similar to other JIRAs, we will rebase this to the HDDS-4454 branch after this has been committed to the master branch. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants