apache
refactor GarbageCollectionLogger so one instance per context
Move GarbageCollectionLogger to ServerContext and start scheduled task in AbstractServer so that the is only one instance running for a server.
Bumping the the spotbugs version in #2824 highlighted a synchronization issue. This change minimizes possibly having multiple instance of GarbageCollectionLogger running as well as making sure one runs per server instance.
While I appreciate the attempt to improve the use of this class, I really think the logic of this class should exist in the TabletServerResourceManager. I am not sure how exactly it would be incorporated with the other memory checks but we have a memoryGuardThread already running in the tserver. This thread is more geared towards Tablet memory usage but I feel like the Java GC checking thread here should be running with the other resource management of the tserver. The GarbageCollectionLogger class feels like it was just created quickly on the fly to try for debugging a complex production issue and just thrown into the tserver.
I really think the logic of this class should exist in the TabletServerResourceManager
The JVM GC Logger is used by all server processes, not just the TabletServer.
I believe that I have addressed the issues as much as can be done without a total refactor. I think it represents an incremental improvement of the current code and larger changes should be handled as a separate issue. Or, just abandon this can stay with what we currently have?
I believe that I have addressed the issues as much as can be done without a total refactor. I think it represents an incremental improvement of the current code and larger changes should be handled as a separate issue. Or, just abandon this can stay with what we currently have?
I am not sure this change is worth the benefit, especially if there is a good chance this class is going to be removed in the future. I would be more in favor of a change that makes it easier to remove the class. Now that I think about it, these changes might make it more difficult to drop the class.
How does this make future changes harder?
- It moves the instantiation of instances to one place.
- Instead of multiple instances running in scheduled executors there is one.
- The instance is created in the AbstractServer code so that any service gets an instance without needed to add code that is not obvious that we use.
How does this make future changes harder?
I didn't say that. I said it makes it more difficult to drop the class. The class is being renamed, moved and its usage refactored. It is being introduced into new classes where it wasn't referenced previously. This PR increases the complexity of the class and its usage. I am advocating for doing nothing and just removing the class. For example... GarbageCollectionLogger is now only referenced 11 times, while the new class is 29.
The rename was a suggestion by @milleruntime
...I think the name of this class is confusing. Some reference to Java or the JVM in the name would greatly help to differentiate it from the Accumulo GC classes. Something like JVMGcLogger or JavaMemoryLogger or JavaGcLogger would be a lot better.
The rename was a suggestion by @milleruntime
...I think the name of this class is confusing. Some reference to Java or the JVM in the name would greatly help to differentiate it from the Accumulo GC classes. Something like JVMGcLogger or JavaMemoryLogger or JavaGcLogger would be a lot better.
Yes, the name is confusing.
I'm also in favor of removing this class, and share @milleruntime 's concern about introducing it in new classes where it wasn't used before. However, I do find the new name less confusing, if we end up keeping it. I also like making it a singleton, if we're keeping it around. We don't need more than one of these per process.
I think we should push this off to 3.0, at least, and decide then what we want to do with it.
This PR will be superseded by the changes with https://github.com/apache/accumulo/pull/3161
This PR will be superseded by the changes with #3161
In that case, I'm closing this, so we can focus on reviewing that one.