fixes #3045 remove stale compactions from coordinator #3059
Conversation
There was a problem hiding this comment.
Looks good, but I wonder if we should put the cleanup in a background thread instead of on the request thread that the Monitor uses to get the data. I'm not sure that this is super-important, as I think the orphaned RUNNING entries happen in a small number of cases. But, if the Monitor is down or never used by the user, then no cleanup will happen.
|
Test seems to fail on JDK 17. Not sure what's going on with that. |
When I first started looking into this I was considering adding this code to the DeadCompactionDetector because it already scans the metadata table and runs periodically in the background. I decided against this because I thought it may make the code harder to understand and would force these two task to run at the same cadence. I think I will take another look at making it run in the background as a task or as part of the dead compaction detector.
I saw that, going to look into it. Seemed to be failing to initialize the class, wonder if thats because I called System.nanoTime to initialize an instance var and maybe JDK 17 does not like that. |
I was able to reproduce this locally and fix it locally in commit 7249313. Was a problem with powermock, reflection, and java modules. Needed to add java.time which contained the Duration class that powermock was trying to modify. |
@dlmarion I modified the code to run periodically in the background. This could avoid an OOME in a long running coordinator process for that case you mentioned. |
Fix resource leak warning introduced in #3059
I added a unit test and I ran all ITs that matched the pattern
*Compaction*ITbut have not tested the scenario in #3045 with these changes. The unit test shows it working in isolation, but not end to end.