[GitHub] [accumulo] cshannon commented on pull request #2778: Add version check to ZooPropStore

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

cshannon commented on PR #2778:
URL: https://github.com/apache/accumulo/pull/2778#issuecomment-1228407091

   I reviewed this as well and it also LTGM. 
   
   This may not be a big deal but my only comment is it would be nice to see a couple tests written for `ConfigRefreshRunnner` to verify the `verifySnapshotVersions()` method does what it is supposed to do after running, either running directly or through an integration test. (Check the cache entries were removed and configs inside ServerConfigurationFactory were cleared, etc). It's possible there are some existing tests that will exercise the change that already exist but I didn't see it with a quick search.
   
   Running it directly would be a little annoying as you would either need to change the scope to package/protected to access it in tests so you could instantiate a new runner and execute `verifySnapshotVersions()` or use reflection.
   
   For an ITest I suppose you could make the refresh interval configurable and set it something to be pretty short (a few seconds maybe) to verify it ran properly and evicted entries.


-- 
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: notifications-unsubscribe@accumulo.apache.org

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