ayushtkn commented on code in PR #5497:
URL: https://github.com/apache/hadoop/pull/5497#discussion_r1146303204
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/driver/TestStateStoreDriverBase.java:
##########
@@ -574,6 +580,26 @@ private static Map<String, Class<?>> getFields(BaseRecord
record) {
return getters;
}
+ public void testCacheLoadMetrics(StateStoreDriver driver, int numRefresh)
+ throws IOException, IllegalArgumentException {
+ StateStoreMetrics metrics = stateStore.getMetrics();
+ final Query<MountTable> query = new Query<>(MountTable.newInstance());
+ driver.getMultiple(MountTable.class, query);
+ final Map<String, MutableRate> cacheLoadMetrics =
metrics.getCacheLoadMetrics();
+ final MutableRate mountTableCache =
cacheLoadMetrics.get("CacheMountTableLoad");
+ assertNotNull("CacheMountTableLoad should be present in the state store
metrics",
+ mountTableCache);
+ // CacheMountTableLoadNumOps
+ final long mountTableCacheLoadNumOps =
mountTableCache.lastStat().numSamples();
+ assertEquals("Num of samples collected should match with " + numRefresh +
". Actual value: "
+ + mountTableCacheLoadNumOps, numRefresh, mountTableCacheLoadNumOps);
+ // CacheMountTableLoadAvgTime
+ final double mountTableCacheLoadAvgTime =
mountTableCache.lastStat().mean();
+ assertTrue(
+ "Mean time duration for cache load is expected to be less than 2000
ms. Actual value: "
+ + mountTableCacheLoadAvgTime, mountTableCacheLoadAvgTime < 2000d);
Review Comment:
Can you drop a comment above explaining the reason why it should be ``less
than 2000 ms``
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/driver/TestStateStoreDriverBase.java:
##########
@@ -574,6 +580,26 @@ private static Map<String, Class<?>> getFields(BaseRecord
record) {
return getters;
}
+ public void testCacheLoadMetrics(StateStoreDriver driver, int numRefresh)
+ throws IOException, IllegalArgumentException {
+ StateStoreMetrics metrics = stateStore.getMetrics();
+ final Query<MountTable> query = new Query<>(MountTable.newInstance());
+ driver.getMultiple(MountTable.class, query);
+ final Map<String, MutableRate> cacheLoadMetrics =
metrics.getCacheLoadMetrics();
+ final MutableRate mountTableCache =
cacheLoadMetrics.get("CacheMountTableLoad");
+ assertNotNull("CacheMountTableLoad should be present in the state store
metrics",
+ mountTableCache);
+ // CacheMountTableLoadNumOps
+ final long mountTableCacheLoadNumOps =
mountTableCache.lastStat().numSamples();
+ assertEquals("Num of samples collected should match with " + numRefresh +
". Actual value: "
+ + mountTableCacheLoadNumOps, numRefresh, mountTableCacheLoadNumOps);
Review Comment:
Do we need to put ``numRefresh`` and ``mountTableCacheLoadNumOps`` in the
message, ``assertEquals`` will print that right, if they aren't equal
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/driver/TestStateStoreZK.java:
##########
@@ -206,4 +206,13 @@ public void testFetchErrors()
stateStoreDriver.setEnableConcurrent(true);
testFetchErrors(stateStoreDriver);
}
+
+ @Test
+ public void testCacheLoadMetrics() throws IOException {
+ getStateStoreService().refreshCaches(true);
+ getStateStoreService().refreshCaches(true);
+ getStateStoreService().refreshCaches(true);
+ testCacheLoadMetrics(getStateStoreDriver(), 4);
Review Comment:
The hard coded number is dependent on other tests in the class, If you add a
test which also does ``refreshCaches``, and then this test is executed this
test will fail.
You need to keep these numbers isolated and safe from changing getting
impacted by addition or change of nature of other tests.
A simple way to try is: Just copy this same test in the same class and run
the entire class, whichever test run late will fail
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
