ZOOKEEPER-3037 - Add JVMPauseMonitor #904

nkalmar · 2019-04-10T12:32:49Z

https://issues.apache.org/jira/browse/ZOOKEEPER-3037

eolivelli

Is this code taken from HBase or another ASF project?

nkalmar · 2019-04-10T14:50:22Z

Yes, JVMPauseMonitor is taken out of Hadoop Common

eolivelli · 2019-04-10T15:17:20Z

ok, I think that we should cite it in the header, like:
https://github.com/apache/bookkeeper/blob/90c79444d6211cd66f4259c04c4e246097821b75/bookkeeper-server/src/test/java/org/apache/bookkeeper/sasl/MiniKdc.java#L67

even if it is an ASF project

eolivelli · 2019-04-10T15:17:34Z

anyway this is a great feature, thanks !

anmolnar · 2019-04-10T16:08:22Z

@nkalmar Please add some description to the pull request or link the Jira ticket.

anmolnar

Overall the code looks good to me, some comments on the testing side:

Currently the tests are not unit tests: they're testing multiple things in a single method. We need to split them.
Please don't use Thread.sleep() in unit tests,
The test file is called JvmPauseMonitorTest, but it's not testing the class that is named of. It tests the configuration parsing logic.
Please categorise your unit tests into separate classes based on the single piece of logic that you're testing, e.g. split them into QuorumPeerMainTest, QuorumPeerConfigTest, ServerConfigTest, etc.
Tests in JvmPauseMonitorTest should validate and only validate code in JvmPauseMonitor and its dependencies nothing else.
Similarly when you test QuorumPeerMain whether it uses JvmPauseMonitor correctly, you should mock JvmPauseMonitor and verify the interaction with mocked methods.

I know that this code has been borrowed from the HBase project, but I'd like to improve the testing side as much as we can without too many modification in the original logic.

eolivelli

Awesome.
I left some comments about the test

zookeeper-server/src/test/java/org/apache/zookeeper/server/util/JvmPauseMonitorTest.java

eolivelli · 2019-04-10T17:44:04Z

zookeeper-server/src/test/java/org/apache/zookeeper/server/util/JvmPauseMonitorTest.java

+        Assert.assertEquals(sleepTime, Long.valueOf(pauseMonitor.sleepTimeMs));
+        Assert.assertEquals(infoTH, Long.valueOf(pauseMonitor.infoThresholdMs));
+
+        Thread.sleep(1000);


It is better to have a loop and wait at most X seconds for the condition to be true.
This way we will reduce flakyness

nkalmar · 2019-04-11T14:02:14Z

Thanks guys, refactored the unit tests. While I'm still using Thread.sleep(), the intention has changed. I think this is OK, but let me know if you have another solution to wait until a condition becomes true. (I know there are 3rd party libraries but I didn't want to introduce a dependency)

edit: I could just have an empty while loop or pull the value from JvmPauseMonitor...

eolivelli

Looks good to me

anmolnar

Looking a lot better now. Just a few more nitpicks please.

anmolnar · 2019-04-12T11:50:40Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/util/JvmPauseMonitor.java

+            }
+        }
+
+        String ret = "Detected pause in JVM or host machine (eg GC): pause of approximately " + extraSleepTime


Use String.format instead?

anmolnar · 2019-04-12T11:51:08Z

zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumPeerConfigTest.java

+        final Long warnTH = 5555L;
+        final Long infoTH = 555L;
+
+        QuorumPeerConfig quorumPeerConfig = new MockQuorumPeerConfig(1);


Please don't use the "mocked" version here, it's not needed.

anmolnar · 2019-04-12T11:51:46Z

zookeeper-server/src/test/java/org/apache/zookeeper/server/util/JvmPauseMonitorTest.java

+        Assert.assertEquals(warnTH, Long.valueOf(pauseMonitor.warnThresholdMs));
+        Assert.assertEquals(infoTH, Long.valueOf(pauseMonitor.infoThresholdMs));
+
+        while(pauseMonitor.getNumGcInfoThresholdExceeded() == 0 && pauseMonitor.getNumGcWarnThresholdExceeded() == 0) {


You might want to || here instead of &&.

anmolnar · 2019-04-12T11:52:40Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/util/JvmPauseMonitor.java

+        monitorThread.start();
+    }
+
+    public void serviceStop() {


This method is only used in the test currently. Please add it to shutdown() methods of ZooKeeperServer and QuorumPeer classes.

anmolnar

+1 lgtm

nkalmar · 2019-04-12T13:53:41Z

retest this please

anmolnar · 2019-04-15T08:21:06Z

retest maven build

anmolnar · 2019-04-18T17:18:23Z

Merged to master. Thanks @nkalmar !

https://issues.apache.org/jira/browse/ZOOKEEPER-3037 Author: Norbert Kalmar <[email protected]> Reviewers: [email protected] Closes apache#904 from nkalmar/ZOOKEEPER-3037 and squashes the following commits: a610532 [Norbert Kalmar] ZOOKEEPER-3037 - add serviceStop() and improve unit tests 7d0baaa [Norbert Kalmar] ZOOKEEPER-3037 - refactor unit tests 97d2c61 [Norbert Kalmar] ZOOKEEPER-3037 - cite hadoop-common as source 3661389 [Norbert Kalmar] ZOOKEEPER-3037 - Add unit test and various improvements f309757 [Norbert Kalmar] ZOOKEEPER-3037 - Add JvmPauseMonitor (cherry picked from commit e9adf6e)

Backporting https://issues.apache.org/jira/browse/ZOOKEEPER-3037 from branch-3.6 to branch-3.5. Author: Norbert Kalmar <nkalmaryahoo.com> Reviewers: andorapache.org Closes #904 from nkalmar/ZOOKEEPER-3037 and squashes the following commits: a610532 [Norbert Kalmar] ZOOKEEPER-3037 - add serviceStop() and improve unit tests 7d0baaa [Norbert Kalmar] ZOOKEEPER-3037 - refactor unit tests 97d2c61 [Norbert Kalmar] ZOOKEEPER-3037 - cite hadoop-common as source 3661389 [Norbert Kalmar] ZOOKEEPER-3037 - Add unit test and various improvements f309757 [Norbert Kalmar] ZOOKEEPER-3037 - Add JvmPauseMonitor (cherry picked from commit e9adf6e) Author: Norbert Kalmar <[email protected]> Reviewers: Enrico Olivelli <[email protected]>, Mate Szalay-Beko <[email protected]>, Andor Molnar <[email protected]> Closes #1594 from symat/ZOOKEEPER-3037-branch-3.5

https://issues.apache.org/jira/browse/ZOOKEEPER-3037 Author: Norbert Kalmar <[email protected]> Reviewers: [email protected] Closes apache#904 from nkalmar/ZOOKEEPER-3037 and squashes the following commits: a610532 [Norbert Kalmar] ZOOKEEPER-3037 - add serviceStop() and improve unit tests 7d0baaa [Norbert Kalmar] ZOOKEEPER-3037 - refactor unit tests 97d2c61 [Norbert Kalmar] ZOOKEEPER-3037 - cite hadoop-common as source 3661389 [Norbert Kalmar] ZOOKEEPER-3037 - Add unit test and various improvements f309757 [Norbert Kalmar] ZOOKEEPER-3037 - Add JvmPauseMonitor

https://issues.apache.org/jira/browse/ZOOKEEPER-3037 Author: Norbert Kalmar <[email protected]> Reviewers: [email protected] Closes apache#904 from nkalmar/ZOOKEEPER-3037 and squashes the following commits: a610532 [Norbert Kalmar] ZOOKEEPER-3037 - add serviceStop() and improve unit tests 7d0baaa [Norbert Kalmar] ZOOKEEPER-3037 - refactor unit tests 97d2c61 [Norbert Kalmar] ZOOKEEPER-3037 - cite hadoop-common as source 3661389 [Norbert Kalmar] ZOOKEEPER-3037 - Add unit test and various improvements f309757 [Norbert Kalmar] ZOOKEEPER-3037 - Add JvmPauseMonitor (cherry picked from commit e9adf6e) Change-Id: I7654fc85168bf163bdbbe96ebf73cb9ccf265e81

ZOOKEEPER-3037 - Add JvmPauseMonitor

f309757

eolivelli reviewed Apr 10, 2019

View reviewed changes

ZOOKEEPER-3037 - Add unit test and various improvements

3661389

nkalmar force-pushed the ZOOKEEPER-3037 branch from 6c07f45 to 3661389 Compare April 10, 2019 15:09

anmolnar requested changes Apr 10, 2019

View reviewed changes

nkalmar changed the title ~~Zookeeper 3037 - Add JVMPauseMonitor~~ Zookeeper-3037 - Add JVMPauseMonitor Apr 10, 2019

nkalmar changed the title ~~Zookeeper-3037 - Add JVMPauseMonitor~~ ZOOKEEPER-3037 - Add JVMPauseMonitor Apr 10, 2019

ZOOKEEPER-3037 - cite hadoop-common as source

97d2c61

eolivelli requested changes Apr 10, 2019

View reviewed changes

ZOOKEEPER-3037 - refactor unit tests

7d0baaa

eolivelli approved these changes Apr 11, 2019

View reviewed changes

anmolnar requested changes Apr 12, 2019

View reviewed changes

anmolnar approved these changes Apr 12, 2019

View reviewed changes

nkalmar force-pushed the ZOOKEEPER-3037 branch from a502086 to ca8aed3 Compare April 12, 2019 14:52

ZOOKEEPER-3037 - add serviceStop() and improve unit tests

a610532

nkalmar force-pushed the ZOOKEEPER-3037 branch from ca8aed3 to a610532 Compare April 12, 2019 14:56

asfgit closed this in e9adf6e Apr 18, 2019

symat mentioned this pull request Feb 5, 2021

ZOOKEEPER-3037: Add JVMPauseMonitor #1594

Closed

nkalmar deleted the ZOOKEEPER-3037 branch October 6, 2022 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-3037 - Add JVMPauseMonitor #904

ZOOKEEPER-3037 - Add JVMPauseMonitor #904

nkalmar commented Apr 10, 2019 •

edited

Loading

eolivelli left a comment

nkalmar commented Apr 10, 2019

eolivelli commented Apr 10, 2019

eolivelli commented Apr 10, 2019

anmolnar commented Apr 10, 2019

anmolnar left a comment •

edited

Loading

eolivelli left a comment

eolivelli Apr 10, 2019

nkalmar commented Apr 11, 2019 •

edited

Loading

eolivelli left a comment

anmolnar left a comment

anmolnar Apr 12, 2019

anmolnar Apr 12, 2019

anmolnar Apr 12, 2019

anmolnar Apr 12, 2019

anmolnar left a comment

nkalmar commented Apr 12, 2019

anmolnar commented Apr 15, 2019

anmolnar commented Apr 18, 2019

ZOOKEEPER-3037 - Add JVMPauseMonitor #904

ZOOKEEPER-3037 - Add JVMPauseMonitor #904

Conversation

nkalmar commented Apr 10, 2019 • edited Loading

eolivelli left a comment

Choose a reason for hiding this comment

nkalmar commented Apr 10, 2019

eolivelli commented Apr 10, 2019

eolivelli commented Apr 10, 2019

anmolnar commented Apr 10, 2019

anmolnar left a comment • edited Loading

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli Apr 10, 2019

Choose a reason for hiding this comment

nkalmar commented Apr 11, 2019 • edited Loading

eolivelli left a comment

Choose a reason for hiding this comment

anmolnar left a comment

Choose a reason for hiding this comment

anmolnar Apr 12, 2019

Choose a reason for hiding this comment

anmolnar Apr 12, 2019

Choose a reason for hiding this comment

anmolnar Apr 12, 2019

Choose a reason for hiding this comment

anmolnar Apr 12, 2019

Choose a reason for hiding this comment

anmolnar left a comment

Choose a reason for hiding this comment

nkalmar commented Apr 12, 2019

anmolnar commented Apr 15, 2019

anmolnar commented Apr 18, 2019

nkalmar commented Apr 10, 2019 •

edited

Loading

anmolnar left a comment •

edited

Loading

nkalmar commented Apr 11, 2019 •

edited

Loading