[GitHub] [accumulo] EdColeman commented on a change in pull request #1745: Fix flakey GcMetricsIT

[GitBox <gi...@apache.org>]

EdColeman commented on a change in pull request #1745:
URL: https://github.com/apache/accumulo/pull/1745#discussion_r509383474



##########
File path: test/src/main/java/org/apache/accumulo/test/functional/GcMetricsIT.java
##########
@@ -98,10 +98,8 @@ public void gcMetricsPublished() {
 
       var updateTimestamp = System.currentTimeMillis();
 
-      // Read two updates, throw away the first snapshot - it could have been from a previous run
-      // or another test (the file appends.)
-      LineUpdate firstUpdate = waitForUpdate(-1, gcTail);
-      firstUpdate = waitForUpdate(firstUpdate.getLastUpdate(), gcTail);
+      // Get next update after current time
+      LineUpdate firstUpdate = waitForUpdate(updateTimestamp, gcTail);

Review comment:
       This looks fine - I do not recall if I had issues and made subsequent changes that makes this work - but I do recall trying to deal with cases where there were multiple runs or multiple tests within a file.  The file tailer would return the last line in the file - and often that was from a different test and that would invalidate any comparisons between metrics updates.  Because these read / parsing the hadoop metrics file, there was not as much control as I would prefer and this was a work-around for those limitations.  
   
   I think the file tailer is used in other metrics tests (FateMetrics) and may benefit from the same changes.
   
   Also, you could argue that these tests are unnecessary - they seem to border on testing the metrics system as much as our functionality - for development they were necessary - for us, the tests could just make sure that the proper values are available to the metrics system, but on the other hand it the test do validate the end-end metrics publishing.  Validating that we publish metrics at some level seems desirable - how much we actually need to then validate the values is where I keep going back and forth.  




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org