Skip to content

Comments

HDDS-4792. Remove redundant state check for handleDeleteChunk#1894

Merged
elek merged 1 commit intoapache:masterfrom
symious:HDDS-4792
Mar 19, 2021
Merged

HDDS-4792. Remove redundant state check for handleDeleteChunk#1894
elek merged 1 commit intoapache:masterfrom
symious:HDDS-4792

Conversation

@symious
Copy link
Contributor

@symious symious commented Feb 4, 2021

What changes were proposed in this pull request?

  • Remove redundant state check for handleDeleteChunk.

What is the link to the Apache JIRA

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

How was this patch tested?

Locally test.

@symious
Copy link
Contributor Author

symious commented Feb 5, 2021

@elek @adoroszlai Could you help to review this PR? Thanks in advance.

checkContainerIsHealthy(kvContainer);
} catch (StorageContainerException sce) {
return ContainerUtils.logAndReturnError(LOG, sce, request);
}
Copy link
Member

Choose a reason for hiding this comment

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

Where is this check available in handleDeleteChunk path? I tried checking callers of the method, couldn't find where this is checked before. Can you point me to that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't get the question? Do you mean where the function is called before?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find the usage of DeleteChunk either.
I think the method of deleteChunk in ChunkManager is designed for FilePerChunkStrategy first that it will only delete one chunk file, in FilePerBlockStrategy the method should throw the exception of unsupported request.

Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

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

+1. Thanks the patch @symious

Checked the code and I agree, this check is redundant.

@elek elek merged commit cc3227c into apache:master Mar 19, 2021
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.

3 participants