Rework cleanup process on shutdown

[pdambrauskas]

fixes #785

Due to this issue DeleteOnExit hook cannot be used on long-running applications.

This PR replaces usage of DeleteOnExitHook with custom shut down hook, which cleans up local storage directory.

@giner @HenryCaiHaiying does this makes sense :) ?

[pdambrauskas]

Only adding this hook, if uploadOnShutdown is disabled, otherwise we cannot be sure that deletion will happen after upload. Java does not provide strong order for shutdown hooks, they can be executed in any order & in parallel.

Other option would be implementing some kind of "shutdown hook registry" combining all hooks into one thread and registring all Runnables as one shutdown hook. But I think we do not need that, since files are deleted after upload anyways.

[HenryCaiHaiying]

Take a look at DeleteOnExitHook.java, there seems a way to register your hook as the last one. If ordering cannot be done, I think you can modify the hook in uploadOnShutdown to have them call your hook's folder deletion methods. It's OK to have both hooks to call delete folders twice.

[HenryCaiHaiying]

Left some comments, it's a good fix. I think we are using deleteOnExit() just in case the normal deletion didn't happen, by looking at FileRegistry.deletePath(), the local file would normally be deleted after upload, otherwise you would see millions of local files on disk during secor running (which would be a out of disk problem rather than out of memory problem).

[HenryCaiHaiying]

Left some comments, it's a good fix. I think we are using deleteOnExit() just in case the normal deletion didn't happen, by looking at FileRegistry.deletePath(), the local file would normally be deleted after upload, otherwise you would see millions of local files on disk during secor running (which would be a out of disk problem rather than out of memory problem).

[HenryCaiHaiying]

Can you rename the class name StagingCleaner to StagingDirectoryCleaner?

[HenryCaiHaiying]

I think we can just call FileUtils.deleteDirectory(stagingPath) instead of using a loop.

[HenryCaiHaiying]

Please use this path instead (Take a look at Upload.java): config.getLocalPath() is a shared folder with other processes/threads running on that host.

                    String localPrefix = mConfig.getLocalPath() + '/' +
                        IdUtil.getLocalMessageDir();

[HenryCaiHaiying]

Take a look at DeleteOnExitHook.java, there seems a way to register your hook as the last one. If ordering cannot be done, I think you can modify the hook in uploadOnShutdown to have them call your hook's folder deletion methods. It's OK to have both hooks to call delete folders twice.

[pdambrauskas]

@HenryCaiHaiying thanks for review!

I've updated shutdown hook logics.

• Renamed delete hook file, as you suggested
• Changed deletion path
• Deleting whole path, instead of files inside
• Created Shutdown hook registry, to make it easier to run hooks in specified order.

I decided not tou use DeleteOnExitHook approch, because documentation of registerShutdownHook method states that it is used only for DeleteOnExitHook. I'm afraid using it somewhere else might break something. I tried it on my first attemt, and integration tests failed in some strange way: https://travis-ci.org/github/pinterest/secor/jobs/706801339

Registers a shutdown hook.
It is expected that this method with registerShutdownInProgress=true
is only used to register DeleteOnExitHook since the first file
may be added to the delete on exit list by the application shutdown
hooks.

Also having single Shut down hook registry looks a bit nicer, as it makes usage of shutdown hooks more visible.

[HenryCaiHaiying]

Looks good, this is a good fix.