-
Notifications
You must be signed in to change notification settings - Fork 62
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
Fix of high cpu usage occurence replicated in Broadcom based on GIT issue #282 #326
Conversation
Codecov Report
@@ Coverage Diff @@
## master #326 +/- ##
============================================
- Coverage 70.02% 69.98% -0.04%
Complexity 12 12
============================================
Files 237 238 +1
Lines 4540 4548 +8
Branches 574 574
============================================
+ Hits 3179 3183 +4
- Misses 1203 1207 +4
Partials 158 158
Continue to review full report at Codecov.
|
404ec5a
to
c43ec67
Compare
I have reworked the solution based on the feedback from Jirka and Lena |
common-service-core/src/main/java/com/ca/mfaas/monitoring/LatencyUtilsConfigInitializer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor issues.
common-service-core/src/main/java/com/ca/mfaas/monitoring/LatencyUtilsConfigInitializer.java
Outdated
Show resolved
Hide resolved
* This class has been | ||
*/ | ||
public class LatencyUtilsConfigInitializer implements ApplicationContextInitializer<ConfigurableApplicationContext> { | ||
private final String PROPERTY_KEY = "LatencyUtils.useActualTime"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this static.
* | ||
* This class has been | ||
*/ | ||
public class LatencyUtilsConfigInitializer implements ApplicationContextInitializer<ConfigurableApplicationContext> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try using @nonnull annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not @NotNull
, it's @Nullable
should be, because, you sending to this method null in test. So, you have two options: create application context object and send to this method, or add this @Nullable
annotation. I would suggest to create some simple common context for this test and use it, for example:
private final ConfigurableApplicationContext ctx = new GenericApplicationContext();
P.s.: wrong line, the correct is 25 :)
common-service-core/src/main/java/com/ca/mfaas/monitoring/LatencyUtilsConfigInitializer.java
Outdated
Show resolved
Hide resolved
* | ||
* This class has been | ||
*/ | ||
public class LatencyUtilsConfigInitializer implements ApplicationContextInitializer<ConfigurableApplicationContext> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not @NotNull
, it's @Nullable
should be, because, you sending to this method null in test. So, you have two options: create application context object and send to this method, or add this @Nullable
annotation. I would suggest to create some simple common context for this test and use it, for example:
private final ConfigurableApplicationContext ctx = new GenericApplicationContext();
P.s.: wrong line, the correct is 25 :)
@Override | ||
public void initialize(ConfigurableApplicationContext applicationContext) { | ||
if (System.getProperties().getProperty(PROPERTY_KEY) == null) | ||
System.getProperties().setProperty(PROPERTY_KEY, "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, make the block with {}
.
assertNull( System.getProperties().getProperty(PROPERTY_KEY) ); | ||
|
||
LatencyUtilsConfigInitializer latencyUtilsConfigInitializer = new LatencyUtilsConfigInitializer(); | ||
latencyUtilsConfigInitializer.initialize(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is that null, and the same in the test below.
LatencyUtilsConfigInitializer latencyUtilsConfigInitializer = new LatencyUtilsConfigInitializer(); | ||
latencyUtilsConfigInitializer.initialize(null); | ||
|
||
assertEquals(System.getProperties().getProperty(PROPERTY_KEY),"false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not correct order, "false"
- is expected value, and should be before actual System.getProperties().getProperty(PROPERTY_KEY)
System.getProperties().remove(PROPERTY_KEY); | ||
String value = "RandomValue"; | ||
System.getProperties().setProperty(PROPERTY_KEY, value); | ||
assertEquals( System.getProperties().getProperty(PROPERTY_KEY), value ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as above: value
- is expected value and should be before actual.
LatencyUtilsConfigInitializer latencyUtilsConfigInitializer = new LatencyUtilsConfigInitializer(); | ||
latencyUtilsConfigInitializer.initialize(null); | ||
|
||
assertEquals( System.getProperties().getProperty(PROPERTY_KEY), value ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi guys,
I just made a quick review.
common-service-core/src/main/java/com/ca/mfaas/monitoring/LatencyUtilsConfigInitializer.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
public void ShouldNotSetSystemPropertyWhenPropertyIsSetFromBefore() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method name is starting with an upper character. Please change to a small one. shouldNotSetSystemPropertyWhenPropertyIsSetFromBefore
private final String PROPERTY_KEY = "LatencyUtils.useActualTime"; | ||
|
||
@Test | ||
public void ShouldSetSystemPropertyWhenPropertyNotSet() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method name is starting with an upper character. Please change to a small one. shouldNotSetSystemPropertyWhenPropertyIsSetFromBefore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor detail ...
public class LatencyUtilsConfigInitializer implements ApplicationContextInitializer<ConfigurableApplicationContext> { | ||
private static final String PROPERTY_KEY = "LatencyUtils.useActualTime"; | ||
@Override | ||
public void initialize(@NonNull ConfigurableApplicationContext applicationContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change it from @NonNull
to @Nonnull
... it will get rid of the warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for a great job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super! Thanks!
…ssue #282 Solution is to switch LatencyUtils.useActualTime off, as described in javadoc of TimeServices class /** * Provide an API for time-related service, such as getting the current time and waiting for * a given period of time. By default, these services are provided by actual time services * in the JDK (i.e. System.nanoTime(), System.currentTimeMillis(), Thread.sleep(), and * java.util.concurrent.locks.LockSupport.parkNanos()). However, if the property * LatencyUtils.useActualTime is set to "false", TimeServers will only move the notion * of time in response to calls to the #setCurrentTime() method. * */ Signed-off-by: janda06 <david.janda@broadcom.com>
Signed-off-by: janda06 <david.janda@broadcom.com>
suppressed security module as there are some classes that triggered the check and this module is soon to be decommissioned changed test method names moved the classes to gateway-common Signed-off-by: janda06 <david.janda@broadcom.com>
Moved gateway-common to apiml-common Signed-off-by: janda06 <david.janda@broadcom.com>
afdc01d
55f54e7
to
afdc01d
Compare
Rebased on master, please re-review. Thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job David!
Solution is to switch LatencyUtils.useActualTime off, as described in javadoc of TimeServices
class
/**
*/
Signed-off-by: janda06 david.janda@broadcom.com