goiri commented on code in PR #5497:
URL: https://github.com/apache/hadoop/pull/5497#discussion_r1142801861


##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/CachedRecordStore.java:
##########
@@ -141,8 +142,9 @@ public boolean loadCache(boolean force) throws IOException {
 
       // Update the metrics for the cache State Store size
       StateStoreMetrics metrics = getDriver().getMetrics();
+      String recordName = getRecordClass().getSimpleName();

Review Comment:
   Shouldn't this be inside of the null check?



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/driver/TestStateStoreDriverBase.java:
##########
@@ -574,6 +578,15 @@ 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);
+    assertEquals(numRefresh,
+        
metrics.getCacheLoadMetrics().get("CacheMountTableLoad").lastStat().numSamples());

Review Comment:
   Extract this a little.
   Probably `metrics.getCacheLoadMetrics()` and the final value.



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/driver/TestStateStoreDriverBase.java:
##########
@@ -574,6 +578,15 @@ 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);
+    assertEquals(numRefresh,

Review Comment:
   Can we check for NumOps and Time?



-- 
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]

Reply via email to