NUTCH-2518 Cleaning up the file system after a job failure. #307

Omkar20895 · 2018-03-27T21:02:51Z

Added a new method cleanupAfterFailure which was already existing in Injector.
I have added this only in one of the 25 instances of job.waitForCompletion(true), please go ahead and review it and I will incorporate the review comments in the upcoming commits/changes.

Confusion:

When a job failure happens do we need to do System.exit(non-zero status) or throw a runtime exception with a custom message specific to each class? I went ahead with the later one. Please suggest otherwise with reason.

Thanks.

lewismc · 2018-03-27T21:16:53Z

src/java/org/apache/nutch/crawl/CrawlDb.java

-      Path outPath = FileOutputFormat.getOutputPath(job);
-      if (fs.exists(outPath))
-        fs.delete(outPath, true);
+      LOG.error("Crawl job failed ", e);


Please always use parameterized logging e.g.
LOG.error("Crawl job failed {}", e);

ACK! I will incorporate this change and send a commit covering all the 25 instances. Thanks.

+1

The CrawlDb cleanup method looks same as that from Injector: maybe move it to CrawlDb.java - there are already methods to lock and install the CrawlDb shared by various jobs (inject, update, merge, generate).

Why not shift the sub routine cleanup to the class nutch/src/java/org/apache/nutch/util/NutchJob.java? This is a nutch job utils file, I think this would be more appropriate. I have cross checked that cleanup sub routine would be useful outside crawldb like in hostdb, indexer etc.

You're right. It's not specific to the CrawlDb because lock file and temp dir are passed explicitly.

Omkar20895 · 2018-03-31T20:07:04Z

I have added a new commit, please feel free to review it and let me know if any changes are required. I will squash the commits at the end.

sebastian-nagel

+1 great job! One point needs a fix. Thanks!

sebastian-nagel · 2018-04-02T16:34:22Z

src/java/org/apache/nutch/crawl/Injector.java

            + job.getStatus().getFailureInfo();
        LOG.error(message);
-        cleanupAfterFailure(tempCrawlDb, lock, fs);
+        CrawlDb.cleanupAfterFailure(tempCrawlDb, lock, fs);


should be NutchJob.cleanupAfterFailure(...)

sebastian-nagel · 2018-04-02T16:34:53Z

src/java/org/apache/nutch/crawl/Injector.java

-      }
-      LockUtil.removeLockFile(fs, lock);
-    } catch (IOException e) {
+      CrawlDb.cleanupAfterFailure(tempCrawlDb, lock, fs);


same as above

I will change this, Thanks.

sebastian-nagel · 2018-04-02T16:39:51Z

src/java/org/apache/nutch/crawl/Generator.java

+              + job.getStatus().getFailureInfo();
+          LOG.error(message);
+          NutchJob.cleanupAfterFailure(tempDir, lock, fs);
+          NutchJob.cleanupAfterFailure(tempDir2, lock, fs);


ok, it's not an error to try to remove the lock file a second time.

No @sebastian-nagel. AFAIK if I try to remove a lock file that does not exist, the function would just return False.

sebastian-nagel · 2018-04-04T10:36:27Z

+1 Thanks, @Omkar20895! I'll step forward and commit the fix.

Omkar20895 · 2018-04-04T10:42:35Z

Thanks @sebastian-nagel :)

NUTCH-2518 Cleaning up the file system after a job failure.

sebastian-nagel · 2018-04-04T10:59:36Z

Squashed, rebased and committed in 8682b96. Thanks again, @Omkar20895!

lewismc · 2018-04-10T22:51:08Z

This is ready to be merged @sebastian-nagel ?

sebastian-nagel · 2018-04-11T10:20:32Z

That's already merged but with a rebase and squash done in command-line. Sorry, that's not automatically detected by github and I forgot to manually close this PR. Thanks!

lewismc reviewed Mar 27, 2018

View reviewed changes

NUTCH-2518 Cleaning up the file system after a job failure.

25b7997

Omkar20895 force-pushed the NUTCH-2518 branch from 9d60ffa to 25b7997 Compare April 2, 2018 11:09

sebastian-nagel requested changes Apr 2, 2018

View reviewed changes

Omkar20895 added 2 commits April 3, 2018 11:44

Merge remote-tracking branch 'nutch/master' into NUTCH-2518

ebe3166

NUTCH-2518 Cleaning up the file system after a job failure

a07b843

sebastian-nagel approved these changes Apr 4, 2018

View reviewed changes

sebastian-nagel added a commit that referenced this pull request Apr 4, 2018

Merge pull request #307 from Omkar20895/NUTCH-2518

8682b96

NUTCH-2518 Cleaning up the file system after a job failure.

sebastian-nagel closed this Apr 11, 2018

NUTCH-2518 Cleaning up the file system after a job failure. #307

NUTCH-2518 Cleaning up the file system after a job failure. #307

Uh oh!

Conversation

Omkar20895 commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastian-nagel Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Omkar20895 Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Omkar20895 commented Mar 31, 2018

Uh oh!

sebastian-nagel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastian-nagel commented Apr 4, 2018

Uh oh!

Omkar20895 commented Apr 4, 2018

Uh oh!

sebastian-nagel commented Apr 4, 2018

Uh oh!

lewismc commented Apr 10, 2018

Uh oh!

sebastian-nagel commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sebastian-nagel Mar 28, 2018 •

edited

Loading

Omkar20895 Mar 28, 2018 •

edited

Loading

sebastian-nagel commented Apr 11, 2018 •

edited

Loading