Skip to content

Conversation

@timmylicheng
Copy link
Contributor

@timmylicheng timmylicheng commented Nov 15, 2019

What changes were proposed in this pull request?

#HDDS-2492
Close pipeline in TestSCMPipelineManager#testPipelineOpenOnlyWhenLeaderReported.

What is the link to the Apache JIRA

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

(Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)

Please replace this section with the link to the Apache JIRA)

How was this patch tested?

UT
(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)

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 @timmylicheng for fixing this.

There is one more place where pipelineManager.close() is missing: in testPipelineCreationFailedMetric. Can you please add that one, too?

Also, testPipelineReport was deleted in #23, but restored (I believe accidentally, when resolving some merge conflict) in #29. Now it is failing, because pipeline health requires leader to be set, but the pipeline in the test has 3 non-leader DNs. If it's simple to fix, can you please also do it in this change? If not, I guess it can be removed again. cc @ChenSammi @swagle

I think commit and PR title have the wrong Jira number, it should be HDDS-2492.

@timmylicheng timmylicheng changed the title HDDS-1492 Fix test clean up issue in TestSCMPipelineManager. HDDS-2492 Fix test clean up issue in TestSCMPipelineManager. Nov 15, 2019
@timmylicheng
Copy link
Contributor Author

Thanks @timmylicheng for fixing this.

There is one more place where pipelineManager.close() is missing: in testPipelineCreationFailedMetric. Can you please add that one, too?

Also, testPipelineReport was deleted in #23, but restored (I believe accidentally, when resolving some merge conflict) in #29. Now it is failing, because pipeline health requires leader to be set, but the pipeline in the test has 3 non-leader DNs. If it's simple to fix, can you please also do it in this change? If not, I guess it can be removed again. cc @ChenSammi @swagle

I think commit and PR title have the wrong Jira number, it should be HDDS-2492.

I will add it.
In terms of testPipelineReport test, https://issues.apache.org/jira/browse/HDDS-2499 will fix the test. The issue was not just the test itself.
Thanks for reviewing! @adoroszlai

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 @timmylicheng for making the additional change.

@ChenSammi ChenSammi merged commit 3ca37b1 into apache:master Nov 15, 2019
@ChenSammi
Copy link
Contributor

+1. Thanks for @timmylicheng, and @adoroszlai for the review.

@timmylicheng timmylicheng deleted the HDDS-2492 branch November 27, 2019 14:24
ptlrs pushed a commit to ptlrs/ozone that referenced this pull request Mar 8, 2025
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