You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by agresch <gi...@git.apache.org> on 2018/10/23 14:04:57 UTC
[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...
GitHub user agresch opened a pull request:
https://github.com/apache/storm/pull/2893
STORM-3272 allow worker-launcher to delete dead symlinks
When a topology folder contains a symlink to a missing file, the worker-launcher would not remove the symlink. This prevents force delete from deleting the folder when cleaning up a topology.
When a cluster is short on resources, this can cause all sorts of issues on the supervisor when rescheduling the same topology when this folder still exists.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/agresch/storm agresch_storm-3272
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2893.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2893
----
commit 131344777ae4cbc4ca47952b3a95f35b11713f23
Author: Aaron Gresch <ag...@...>
Date: 2018-10-23T13:57:52Z
STORM-3272 allow worker-launcher to delete dead symlinks
----
---
[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on a diff in the pull request:
https://github.com/apache/storm/pull/2893#discussion_r227434073
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -593,14 +593,17 @@ int recursive_delete(const char *path, int supervisor_owns_dir) {
return UNABLE_TO_BUILD_PATH;
}
+ struct stat file_stat;
+
if(access(path, F_OK) != 0) {
if(errno == ENOENT) {
- return 0;
- }
- // Can probably return here, but we'll try to lstat anyway.
- }
+ // we need to handle symlinks that target missing files.
+ if((lstat(path, &file_stat) != 0) || ((file_stat.st_mode & S_IFMT) != S_IFLNK)) {
+ return 0;
+ }
--- End diff --
@d2r @revans2 - added logging
---
[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/2893#discussion_r227445654
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -593,14 +593,17 @@ int recursive_delete(const char *path, int supervisor_owns_dir) {
return UNABLE_TO_BUILD_PATH;
}
+ struct stat file_stat;
+
if(access(path, F_OK) != 0) {
if(errno == ENOENT) {
- return 0;
- }
- // Can probably return here, but we'll try to lstat anyway.
- }
+ // we need to handle symlinks that target missing files.
+ if((lstat(path, &file_stat) != 0) || ((file_stat.st_mode & S_IFMT) != S_IFLNK)) {
+ return 0;
+ }
--- End diff --
Thanks. And I like that you have text that differentiates this from the other `lstat` error case.
---
[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2893#discussion_r227418284
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -593,14 +593,17 @@ int recursive_delete(const char *path, int supervisor_owns_dir) {
return UNABLE_TO_BUILD_PATH;
}
+ struct stat file_stat;
+
if(access(path, F_OK) != 0) {
if(errno == ENOENT) {
- return 0;
- }
- // Can probably return here, but we'll try to lstat anyway.
- }
+ // we need to handle symlinks that target missing files.
+ if((lstat(path, &file_stat) != 0) || ((file_stat.st_mode & S_IFMT) != S_IFLNK)) {
+ return 0;
+ }
--- End diff --
+1 to logging the failure. The only time we get an ENOENT and lstat fails is either there was a race that the file was deleted out from under us (which should be rare so logging it is fine) or if something truly bad happened, and we have another problem we need to debug.
---
[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/2893#discussion_r227412685
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -593,14 +593,17 @@ int recursive_delete(const char *path, int supervisor_owns_dir) {
return UNABLE_TO_BUILD_PATH;
}
+ struct stat file_stat;
+
if(access(path, F_OK) != 0) {
if(errno == ENOENT) {
- return 0;
- }
- // Can probably return here, but we'll try to lstat anyway.
- }
+ // we need to handle symlinks that target missing files.
+ if((lstat(path, &file_stat) != 0) || ((file_stat.st_mode & S_IFMT) != S_IFLNK)) {
+ return 0;
+ }
--- End diff --
If we fail to `lstat` the path, do we want to log a message as we do below?
---
[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2893
---