Skip to content

Conversation

@umamaheswararao
Copy link
Contributor

What changes were proposed in this pull request?

Handled the case when chunk list null, but getChunks internal method returning empty list. In this case, we have made wrong assumption and access array elements. This fixed the issue and added the necessary checks.

What is the link to the Apache JIRA

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

How was this patch tested?

Added the test.

Copy link
Member

@kaijchen kaijchen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @umamaheswararao. Please fix the style issue in CI.

}

@Test
void testECReconstructionWithPartialStripe()
Copy link
Member

Choose a reason for hiding this comment

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

Hi @umamaheswararao, the fix looks good.
However, seems the test here is not covering the bug.
Could you please take another look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2023-02-08 at 8 39 49 AM

It is failing when I run this test.
Interestingly I noticed that, when I run the tests together with testECReconstructionCoordinatorWithMissingIndexes135, it passes. Can you try running this test alone?

Copy link
Member

@kaijchen kaijchen Feb 8, 2023

Choose a reason for hiding this comment

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

It is failing when I run this test. Interestingly I noticed that, when I run the tests together with testECReconstructionCoordinatorWithMissingIndexes135, it passes. Can you try running this test alone?

Confirmed. Maybe there is interference among the tests here?

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 figured out the problem. Test indeed tests the behavior, however because of using the inputchunks array which was might created by other tests, number of bytes written to file was more than one chunk. So, it's not meeting the expected situation when ran with other tests. If you run this test alone, it will consistently fail as input chunk initialization will happen as expected, that is just 1 chunk. I will update the patch shortly.

@kaijchen kaijchen merged commit b5230a2 into apache:master Feb 9, 2023
@kaijchen
Copy link
Member

kaijchen commented Feb 9, 2023

Merged, thanks @umamaheswararao and @adoroszlai.

hemantk-12 pushed a commit to hemantk-12/ozone that referenced this pull request Feb 16, 2023
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.

4 participants