Fix Race in jar upload during hadoop indexing #1815

nishantmonu51 · 2015-10-09T18:50:41Z

fixes Race while uploading jar files.

SNAPSHOT jar files are always uploaded to job specific non-shared directory and cleaned up with the job cleanup.
Non-Snapshot jar files are first uploaded to temporary intermediate path and then renamed to the shared directory without overwriting any existing file. this fixes the race when multiple jobs sees the jar file as not present and try to upload it simultaneously.

Fixes - #582

drcrallen · 2015-10-09T22:48:33Z

indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java

+      uploadJar(jarFile, intermediateHdfsPath, fs);
+      try {
+        log.info("Renaming jar to path[%s]", hdfsPath);
+        fs.rename(intermediateHdfsPath, hdfsPath);


If this races can it leave intermediateHdfsPath stale in the distributed FS?

the intermediate path is in job working directory which gets cleaned up during job cleanup.
Also, fixed it to clean the intermediate jar file here.

drcrallen · 2015-10-13T18:27:41Z

indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java

+          throw e;
+        }
+      } finally {
+        if (fs.exists(intermediateHdfsPath)) {


Exists and delete can throw IOException, you'll want to catch those and at least log them. This is a bit tricky

ideally the exceptions would bubble up as suppressed exceptions, but that may not be possible to do in a clean way.

drcrallen · 2015-10-13T20:52:03Z

indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java

      }
    }
+    DistributedCache.addFileToClassPath(hdfsPath, conf, fs);


DistributedCache is deprecated, can you use job.addFileToClassPath to add the files?

Yes, right now that seems to do exactly this under the hood, but if hadoop ever gets their crap together they can change this without warning.

xvrl · 2015-10-14T16:56:23Z

@nishantmonu51 @drcrallen looks like this change is a little less trivial than originally thought. Should we push it back to 0.8.3 / 0.9.0?

drcrallen · 2015-10-14T17:00:36Z

indexing-hadoop/src/main/java/io/druid/indexer/updater/HadoopConverterJob.java

  public static void cleanup(Job job) throws IOException
  {
    final Path jobDir = getJobPath(job.getJobID(), job.getWorkingDirectory());
    final FileSystem fs = jobDir.getFileSystem(job.getConfiguration());
    fs.delete(jobDir, true);
+    fs.delete(getJobClassPathDir(job.getJobName(), job.getWorkingDirectory()), true);


This method could use some extra assurances / retries as well. (non-blocking)

I think we need to do this for cleanup for other Hadoop Jobs as well, would like to do it in a separate PR. Opening a github issue

drcrallen · 2015-10-14T17:02:07Z

@xvrl I'm actually ok with it right now. I would love to have some more improvements on it and Unit tests, which can come in 0.9.x but at this point I think it is in a much better place than it was, and I'm 👍 if @nishantmonu51 is willing to add some more assurances and unit tests for 0.9

nishantmonu51 · 2015-10-14T17:15:48Z

I tried to write a Unit test for above, but the JobHelper.setupClasspath skips the jar copying if the fileSystem is LocalFileSystem, so i tested it manually with local HDFS setup.
For adding retry logic for task cleanups, I think we can add that in 0.9, will create an issue for doing that in a separate PR.

nishantmonu51 · 2015-10-14T17:22:39Z

btw, I am fine with moving this to 0.9/0.8.3 also.

drcrallen · 2015-10-14T17:28:41Z

@nishantmonu51 io.druid.segment.loading.HdfsFileTimestampVersionFinderTest#setupStatic sets up a mini hdfs cluster for actual hdfs testing.

nishantmonu51 · 2015-10-14T17:35:42Z

@drcrallen thanks, I will use that and add a Test.

himanshug · 2015-10-15T03:52:35Z

indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java

+  /**
+   * Uploads jar files to hdfs and configures the classpath.
+   * Snapshot jar files are uploaded to intermediateClasspath and not shared across multiple jobs.
+   * Non-Snapshot


Non-Snapshot? Is this incomplete sentence?

nishantmonu51 · 2015-10-15T18:02:11Z

@drcrallen added test.

himanshug · 2015-10-15T18:15:02Z

indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java

        }
+        return apply(input.getCause());


addJarToClassPath has

throw new ISE("File does not exist even after moving from[%s] to [%s]", intermediateHdfsPath, hdfsPath);

that means it won't be retried. Is that intentional?

why not retry here irrespective of what the exception type is, in the worst case there would be 3 retries instead of having to do a recursion?

will modify it to IOException though this should ideally never happen in any case.

himanshug · 2015-10-15T19:12:34Z

indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java

@@ -79,6 +79,8 @@
  private static final int NUM_RETRIES = 8;
  private static final int SECONDS_BETWEEN_RETRIES = 2;
  private static final int DEFAULT_FS_BUFFER_SIZE = 1 << 18; // 256KB
+  private static final Pattern SNAPSHOT_JAR = Pattern.compile(".*SNAPSHOT(-selfcontained)?\\.jar$");


nit: this should be

Pattern.compile(".*\\-SNAPSHOT(-selfcontained)?\\.jar$")

just in case someone was crazy enough to have artifact name be something like iAmNotSNAPSHOT.jar , it is not a "snapshot" jar really.

himanshug · 2015-10-16T14:48:39Z

LGTM

guobingkun · 2015-10-16T22:50:26Z

indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java

+          if (exception != null) {
+            exception.addSuppressed(e);
+          } else {
+            exception = e;


Is this just making sure we don't have a jar left over at intermediateHdfsPath?

I see :) Can we also invert the condition in "if else"? I actually feel it takes a bit less mind gymnastics to understand if it was

if (exception == null) { exception = e; } else { exception.addSuppressed(e); }

drcrallen · 2015-10-20T17:51:04Z

indexing-hadoop/src/test/java/io/druid/indexer/HdfsClasspathSetupTest.java

+  {
+    Job job = Job.getInstance(conf, "test-job");
+    DistributedFileSystem fs = miniCluster.getFileSystem();
+    Path intermediatePath = new Path("/tmp/classpath");


Intermediate path should be unique per task, right? can this be randomized a bit so one test cannot pollute another?

same here, this is on hdfs, diff runs will have diff hdfs directory.

drcrallen · 2015-10-20T17:54:30Z

@nishantmonu51 is there a way to add tests for failure cases?

guobingkun · 2015-10-21T18:51:06Z

indexing-hadoop/src/test/java/io/druid/indexer/HdfsClasspathSetupTest.java

+                  int id = barrier.await();
+                  Job job = Job.getInstance(conf, "test-job-" + id);
+                  Path intermediatePathForJob = new Path(intermediatePath, "job-" + id);
+                  barrier.await();


not sure why we need to await() for the second time?

nishantmonu51 · 2015-10-21T20:07:18Z

I added a test for concurrent upload which tries to simulate the failure case which this PR is intended to fix.

drcrallen · 2015-10-21T20:35:40Z

indexing-hadoop/src/test/java/io/druid/indexer/HdfsClasspathSetupTest.java

+      );
+    }
+
+    for (Future<Boolean> future : futures) {


suggest Futures.allAsList(futures).get(....)

Then you don't really need to check the return value because it should bubble up any exceptions in the execution.

nice suggestion, done.

drcrallen · 2015-10-22T14:39:43Z

👍

himanshug · 2015-10-22T15:55:14Z

indexing-hadoop/src/test/java/io/druid/indexer/HdfsClasspathSetupTest.java

+  {
+    hdfsTmpDir = File.createTempFile("hdfsClasspathSetupTest", "dir");
+    hdfsTmpDir.deleteOnExit();
+    if (!hdfsTmpDir.delete()) {


why is hdfsTmpDir.deleteOnExit() needed when hdfsTmpDir.delete() is ensured?
Also, it is safer to use @rule TemporaryFolder instead as that will do cleanup as soon as test is done instead of jvm exit.

I tried using TemporaryFolder, but we cant use it as a static field.

hmmm, if this is to be done only once for all the tests here then we can probably make things non static and use a no argument constructor to do the setup? Is there a reason for hdfsTmpDir to be static?

I can use a non arg constructor, but still i will need to make the fields static since i will have to shut it down in a static afterClass method. I think it will be wierd to initialize static fields of a class in constructor rather than a BeforeClass method.

ok, i see, junit unfortunately does not have anything to setup/teardown "instance" .

few fixes delete intermediate file early better exception handling use static pattern instead of compiling it every time Add retry for transient exceptions remove usage of deprecated method. Add test fix imports fix javadoc review comment. review comment: handle crazy snapshot naming review comments remove default retry count in favour of already present constant review comment make random intermediate and final paths. review comment, use temporaryFolder where possible

Fix Race in jar upload during hadoop indexing

nishantmonu51 changed the title ~~Fix Race in jar upload during hadoop indexing - https://github.com/dr…~~ Fix Race in jar upload during hadoop indexing Oct 9, 2015

drcrallen reviewed Oct 9, 2015
View reviewed changes

nishantmonu51 force-pushed the jar-upload-race branch from 729c33c to bf54991 Compare October 10, 2015 17:16

xvrl added this to the 0.8.2 milestone Oct 13, 2015

drcrallen reviewed Oct 13, 2015
View reviewed changes

nishantmonu51 force-pushed the jar-upload-race branch 2 times, most recently from f86b5f6 to 31bf270 Compare October 13, 2015 18:54

drcrallen reviewed Oct 13, 2015
View reviewed changes

drcrallen reviewed Oct 14, 2015
View reviewed changes

nishantmonu51 mentioned this pull request Oct 14, 2015

Retry for transient exceptions while doing cleanup for Hadoop Jobs #1828

Closed

xvrl modified the milestones: 0.8.3, 0.8.2 Oct 14, 2015

himanshug reviewed Oct 15, 2015
View reviewed changes

nishantmonu51 force-pushed the jar-upload-race branch 2 times, most recently from fa3753f to fca3250 Compare October 15, 2015 17:59

himanshug reviewed Oct 15, 2015
View reviewed changes

nishantmonu51 force-pushed the jar-upload-race branch from fca3250 to 3636aa9 Compare October 15, 2015 18:23

himanshug reviewed Oct 15, 2015
View reviewed changes

nishantmonu51 force-pushed the jar-upload-race branch from 3636aa9 to 62bef6b Compare October 16, 2015 06:41

guobingkun reviewed Oct 16, 2015
View reviewed changes

nishantmonu51 modified the milestones: 0.9.0, 0.8.3 Oct 20, 2015

drcrallen reviewed Oct 20, 2015
View reviewed changes

guobingkun reviewed Oct 21, 2015
View reviewed changes

nishantmonu51 force-pushed the jar-upload-race branch 2 times, most recently from 9ba4f83 to 41cc078 Compare October 21, 2015 20:05

drcrallen reviewed Oct 21, 2015
View reviewed changes

nishantmonu51 force-pushed the jar-upload-race branch 2 times, most recently from d457699 to 31097f5 Compare October 22, 2015 06:12

himanshug reviewed Oct 22, 2015
View reviewed changes

nishantmonu51 force-pushed the jar-upload-race branch from 31097f5 to 3641a0e Compare October 22, 2015 16:11

himanshug added a commit that referenced this pull request Oct 23, 2015

Merge pull request #1815 from metamx/jar-upload-race

2c3753c

Fix Race in jar upload during hadoop indexing

himanshug merged commit 2c3753c into apache:master Oct 23, 2015

himanshug mentioned this pull request Oct 26, 2015

fixing hadoop test scope dependencies in indexing-hadoop #1867

Merged

drcrallen deleted the jar-upload-race branch October 26, 2015 22:29

gianm modified the milestones: 0.8.3, 0.9.0 Dec 1, 2015

This was referenced Dec 1, 2015

0.8.3 backports #2022

Merged

druid-0.8.3 release notes #2044

Closed

Fix Race in jar upload during hadoop indexing #1815

Fix Race in jar upload during hadoop indexing #1815

Conversation

nishantmonu51 commented Oct 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xvrl commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drcrallen commented Oct 14, 2015

nishantmonu51 commented Oct 14, 2015

nishantmonu51 commented Oct 14, 2015

drcrallen commented Oct 14, 2015

nishantmonu51 commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishantmonu51 commented Oct 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshug commented Oct 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drcrallen commented Oct 20, 2015

Choose a reason for hiding this comment

nishantmonu51 commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drcrallen commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment