Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HDDS-3549. TestKeyInputStream#testSeek fails intermittently. #899

Merged
merged 1 commit into from May 13, 2020

Conversation

lokeshj1703
Copy link
Contributor

@lokeshj1703 lokeshj1703 commented May 6, 2020

What changes were proposed in this pull request?

  1. The test TestKeyInputStream#testSeek compares incorrect metric. This metric compared pendingOps.
    Assert.assertEquals(readChunkCount, metrics .getContainerOpsMetrics(ContainerProtos.Type.ReadChunk));
    The PR now compares the correct metric i.e. getContainerOpCountMetrics.
  2. Changed name for metric value getter to make it more distinct. From getContainerOpsMetrics to getPendingContainerOpCountMetrics.

What is the link to the Apache JIRA

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

How was this patch tested?

The patch fixes a unit test.

bshashikant
bshashikant previously approved these changes May 6, 2020
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @lokeshj1703 for working on this. Renaming the methods to clarify their difference is a good change. But I don't think the mismatch causes the test failure, both metrics should be 0 at that point.

Copy link
Contributor

@bshashikant bshashikant left a comment

Choose a reason for hiding this comment

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

XCeiverClientMetrics is shared instance among all grpc clients and the increment/decrement not being atomic might end up resulting in negative value.

@bshashikant bshashikant self-requested a review May 7, 2020 07:44
@bshashikant bshashikant dismissed their stale review May 7, 2020 07:44

Needs more changes.

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 @lokeshj1703 fix it.

I propose to commit this patch and fix the other (real?) problem in a separated Jira, if you agree (@bshashikant , @adoroszlai ) as this would fix intermittency in our tests immediately.

@xiaoyuyao
Copy link
Contributor

+1 to commit this. Do we have a separate follow up JIRA?

@lokeshj1703
Copy link
Contributor Author

Filed https://issues.apache.org/jira/browse/HDDS-3578 to fix the other issue.

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, the patch looks good to me.

@mukul1987 mukul1987 merged commit be8a2fc into apache:master May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants