Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-9027] [web] Clean up web UI resources by installing shut down hook #5740

Closed
wants to merge 2 commits into from

Conversation

tillrohrmann
Copy link
Contributor

What is the purpose of the change

The RestServerEndpoint creates temp directories to upload files and store the web ui.
In case of a hard shut down, as it happens with bin/stop-cluster.sh we should still
delete these files. We do this by installing a shut down hook.

Verifying this change

  • Manually tested

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@StephanEwen
Copy link
Contributor

The TaskManagers have one (or more) root directories for all temp files. They only have one shutdown hook that removes them at the very end. No separate hooks in the separate components.

Should we try and have the same in the JobManager?

Aside form that, I think previously users liked that the jar upload directory was not cleaned by default when they explicitly configured it. Does this change preserve the behavior?

@tillrohrmann
Copy link
Contributor Author

I agree that having a single shut down hook for cleaning up the temp directory is the best solution. I think that the TaskManager does not properly do it like you've described. The TaskExecutorLocalStateStoresManager registers its own shut down hooks and most of the TaskManager components like BlobCache, FileCache and IOManager do so likewise.

The upload directory is a good point. With my changes it will always be deleted. If this is not the intended behaviour, then I will adapt it accordingly.

In order to not widen the scope of this task too much I would suggest the following:

  1. Addressing the general problem of reducing the number of shut down hooks in a separate issue
  2. Creating a a sub directory in WebOptions#TMP_DIR which is set up and cleaned up by the ClusterEntrypoint and its shut down hook

What do you think @StephanEwen.

@StephanEwen
Copy link
Contributor

Okay, seems like the TaskManager design diverged from the "single temp dir" assumption we had originally.

How about creating the upload dir inside the web temp dir by default (then it is deleted), but not delete it explicitly (in which case it stays if users explicitly configure it to a different location)?

…hook

The ClusterEntrypoint creates temp directory for the RestServerEndpoint. This
directory contains the web ui files and if not differently configured the web
upload directory. In case of a hard shut down, as it happens with bin/stop-cluster.sh
the ClusterEntrypoint will clean up this directory by installing a shut down hook.

All future directory cleanup tasks should go into this method
ClusterEntrypoin#cleanupDirectories.
@tillrohrmann
Copy link
Contributor Author

Sounds like a good idea.

@tillrohrmann
Copy link
Contributor Author

Thanks for the feedback @StephanEwen. Merging this PR.

tillrohrmann added a commit to tillrohrmann/flink that referenced this pull request Mar 23, 2018
…hook

The ClusterEntrypoint creates temp directory for the RestServerEndpoint. This
directory contains the web ui files and if not differently configured the web
upload directory. In case of a hard shut down, as it happens with bin/stop-cluster.sh
the ClusterEntrypoint will clean up this directory by installing a shut down hook.

All future directory cleanup tasks should go into this method
ClusterEntrypoin#cleanupDirectories.

This closes apache#5740.
@asfgit asfgit closed this in e180558 Mar 23, 2018
asfgit pushed a commit that referenced this pull request Mar 23, 2018
…hook

The ClusterEntrypoint creates temp directory for the RestServerEndpoint. This
directory contains the web ui files and if not differently configured the web
upload directory. In case of a hard shut down, as it happens with bin/stop-cluster.sh
the ClusterEntrypoint will clean up this directory by installing a shut down hook.

All future directory cleanup tasks should go into this method
ClusterEntrypoin#cleanupDirectories.

This closes #5740.
@tillrohrmann tillrohrmann deleted the cleanupWebUI branch May 29, 2018 10:16
sampathBhat pushed a commit to sampathBhat/flink that referenced this pull request Jul 26, 2018
…hook

The ClusterEntrypoint creates temp directory for the RestServerEndpoint. This
directory contains the web ui files and if not differently configured the web
upload directory. In case of a hard shut down, as it happens with bin/stop-cluster.sh
the ClusterEntrypoint will clean up this directory by installing a shut down hook.

All future directory cleanup tasks should go into this method
ClusterEntrypoin#cleanupDirectories.

This closes apache#5740.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants