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.

Sign up for GitHub

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

Jump to bottom

HDDS-3248. shutdown defaultMetricsSystem before tests #704

Merged
merged 6 commits into from Mar 31, 2020
Merged

HDDS-3248. shutdown defaultMetricsSystem before tests #704

merged 6 commits into from Mar 31, 2020

Conversation

isahkemat
Copy link
Contributor

@isahkemat isahkemat commented Mar 23, 2020 •
edited

What changes were proposed in this pull request?

clean up DefaultMetricSystem after TestHandler and TestHddsDispatcher

What is the link to the Apache JIRA

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

@fapifta
Copy link
Contributor

fapifta commented Mar 23, 2020

Thank you for figuring this out, and posting a JIRA about it.

As in #705 I disagree with how the solution is implemented.
The main problem with the approach is that we are fixing a code which is not broken because some other code does not do the necessary cleanup...

(also the PR template is missing here as well, would be good to include it)

@isahkemat
Copy link
Contributor Author

Hi, you're right. we should clean up after tests, not before them. and I change this PR to use this approach. (I also edit the PR description to follow PR template)

@isahkemat
Copy link
Contributor Author

I don't understand why it-freon failed, I ran this command in my environment and it finished successfully

./hadoop-ozone/dev-support/checks/integration.sh -Pfreon
...
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Apache Hadoop Ozone Integration Tests 0.5.0-SNAPSHOT:
[INFO] 
[INFO] Apache Hadoop Ozone Integration Tests .............. SUCCESS [07:50 min]
[INFO] Apache Hadoop Ozone Mini Ozone Chaos Tests ......... SUCCESS [  1.363 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

fapifta
fapifta reviewed Mar 24, 2020
View reviewed changes
...-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestHddsDispatcher.java
@@ -132,6 +132,7 @@ public void testContainerCloseActionWhenFull() throws IOException {

} finally {
volumeSet.shutdown();
ContainerMetrics.remove();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call shutdownDispatcher() here as well instead of just ContainerMetrics.remove()?

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 removed that method and unify all the calls to ContainerMetrics.remove()

fapifta
fapifta reviewed Mar 24, 2020
View reviewed changes
...-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestHddsDispatcher.java Outdated
if(dispatcher != null) {
dispatcher.shutdown();
}
ContainerMetrics.remove();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not convinced whether this step to remove the ContainerMetrics belongs to a method called shutdownDispatcher, at least from the name it is not clear it will do so besides shutting down the dispatcher. Shouldn't we separate this two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually dispatcher.shutdown() method do nothing, and I put it here to follow the principle of shutdown used objects.
I will edit this part to just call ContainerMetrics.remove()

fapifta
fapifta reviewed Mar 24, 2020
View reviewed changes
...ice/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolumeChecker.java Outdated
@@ -91,6 +92,7 @@

@Before
public void setup() throws IOException {
DefaultMetricsSystem.instance().shutdown();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this one after the added cleanup in TestHddsDispatcher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, I will remove this

@fapifta
Copy link
Contributor

fapifta commented Mar 24, 2020

Thank you for the follow up, and addressing my concern, I have added a few inline comments, can you please check to the questions there?

@fapifta
Copy link
Contributor

fapifta commented Mar 24, 2020

The changeset this way looks good to me, thank you for addressing the concerns, if it runs fine I am +1 on it (non-binding)

@isahkemat
Copy link
Contributor Author

Thank you for guiding me through this. :)

@mukul1987 mukul1987 merged commit 97f59f8 into apache:master Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fapifta fapifta fapifta left review comments

@mukul1987 mukul1987 mukul1987 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@isahkemat @fapifta @mukul1987