Fix race between canHandle() and addSegment() in StorageLocation by jihoonson · Pull Request #8114 · apache/druid

jihoonson · 2019-07-19T22:19:36Z

Description

Historicals can use multiple threads to load segments in parallel from deep storage now (#7088, #4966). This could lead an incorrect computation of remaining space in StorageLocation.

The current pattern to use StorageLocation is:

if (location.canHandle(segment)) {
  // load segment files into the location
  location.addSegment(segment);
}

Even though each of canHandle() and addSegment() is synchronized, they are not atomically executed which could lead a wrong estimation of the available space.

This PR fixes this problem to add a new method, reserve(), to StorageLocation. reserve() basically does what canHandle() and addSegment() atomically. If some error occurs after reserve(), the caller should call remove(segment) to release the reserved space.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added unit tests or modified existing tests to cover new code paths.

leventov · 2019-07-20T07:51:24Z

@jihoonson please don't remove "this PR has been reviewed with concurrency checklist" item from the list since this PR does have relation to concurrency.

jihoonson · 2019-07-21T05:03:46Z

@leventov added back.

clintropolis

overall lgtm 👍

clintropolis · 2019-07-23T05:51:10Z


-  public boolean canHandle(DataSegment segment)
+  /**
+   * This method is available for only unit tests. Production code must use {@link #reserve} instead.


This javadoc is semi confusing because reserve calls canHandle?

Tried to make it clear.

clintropolis · 2019-07-23T05:57:12Z

 * In native parallel indexing, phase 1 tasks store segment files in local storage of middleManagers
 * and phase 2 tasks read those files via HTTP.
- *
+ * <p>


nit: were these <p> added accidentally?

Hmm, these are actually inserted by Intellij's auto indentation based on the code style. It seems legit?

Hmm, I think there is maybe some reason why this isn't in the code style - i remember being told to remove these in the past and to manually adjust intellij to not insert them, I'll see if I can find the reason.

I think the reason we usually avoid them is that they look silly, and also aren't adding anything useful since Druid javadocs are mostly read in source form anyway. It's not a big deal either way. Maybe one day someone will add a style rule for it and get rid of them all.

👍 Removed it.

clintropolis · 2019-07-23T06:23:19Z

+   * Remove a segment dir from this location. The segment size is subtracted from currSize.
   */
-  public synchronized void addSegmentDir(File segmentDir, DataSegment segment)
+  public synchronized void removeSegmentDir(File segmentDir, DataSegment segment)


After thinking about it a bit, it doesn't seem particularly beneficial for StorageLocation to have to be aware of DataSegment and SegmentId, since DataSegment is only used as a shorthand to get the size and SegmentId, and the SegmentId is only used for logging. It seems like reworking this might make the implementation of StorageLocation a bit more simple and allow IntermediaryDataManager and SegmentLoaderLocalCacheManager to use the same reserve and remove methods. I would consider this optional however, up to you if you want to investigate.

I already looked into that way. The thing is, the way to use StorageLocation is different in IntermediaryDataManager and SegmentLoaderLocalCacheManager. In IntermediaryDataManager, the compressed segment files are stored and registered in StorageLocation. In SegmentLoaderLocalCacheManager, the uncompressed segment directories are registered in StorageLocation. This led to use different size computations in removeFile(File) and removeSegmentDir(File, DataSegment). I wanted to make sure that the caller must be aware of what it's registering and call the right method.

I guess we could have an abstract StorageLocation class and its two implementations for different use cases. But I'm not sure it's super beneficial at this point because StorageLocation is still pretty simple.

Thanks for the explanation, sgtm 👍

leventov · 2019-07-23T15:47:13Z

+            return;
+          }
+        }
+        catch (Exception e) {


Can catch narrower exception type?

This is to print something in the log file so that operators could notice that something happened.

This is generally considered an antipattern to catch everything just in case unless the exception is rethrown in the catch block. Could catch only IOExceptions?

Sorry, I was confused with what part you were saying. It sounds good. Fixed.

leventov · 2019-07-23T15:47:39Z

+          }
+        }
+        catch (Exception e) {
+          // Print only log here to try other locations as well.


"Print only log" - a strange phrase. Perhaps it should be just "Only log"

Thanks, fixed.

leventov · 2019-07-23T15:48:40Z


  private final File path;
-  private final long maxSize;
+  private final long maxSize; // in bytes


Can call this field maxSizeBytes

leventov · 2019-07-23T15:50:32Z

+  private final long maxSize; // in bytes
  private final long freeSpaceToKeep;
+
+  // Set of files stored under the given path. All accesses must be synchronized with currSize.


Please turn this comment into a Javadoc comment.

leventov · 2019-07-23T15:50:51Z

  private final Set<File> files = new HashSet<>();

-  private volatile long currSize = 0;
+  // Current total size of files in bytes. All accesses must be synchronized with files.


leventov · 2019-07-23T15:51:27Z


-  private volatile long currSize = 0;
+  // Current total size of files in bytes. All accesses must be synchronized with files.
+  private long currSize = 0;


"All accesses must be synchronized with files." can be replaced with @GuardedBy("this"). Other fields in this class should be annotated, too. See https://github.com/code-review-checklists/java-concurrency#guarded-by

Can call this field currSizeBytes

Good convention. Added.

leventov · 2019-07-23T15:53:41Z

+  boolean canHandle(SegmentId segmentId, long segmentSize)
  {
-    if (available() < segment.getSize()) {
+    if (available() < segmentSize) {


I suggest to call this method "availableSizeBytes"

leventov · 2019-07-23T15:55:03Z

+   * This method is available for only unit tests. Production code must use {@link #reserve} instead.
+   */
+  @VisibleForTesting
+  boolean canHandle(SegmentId segmentId, long segmentSize)


This method should be annotated @GuardedBy("this"). See https://github.com/code-review-checklists/java-concurrency#guarded-by

clintropolis

lgtm 👍

clintropolis · 2019-07-24T00:29:17Z

+   * Remove a segment dir from this location. The segment size is subtracted from currSize.
   */
-  public synchronized void addSegmentDir(File segmentDir, DataSegment segment)
+  public synchronized void removeSegmentDir(File segmentDir, DataSegment segment)


Thanks for the explanation, sgtm 👍

jihoonson · 2019-07-26T16:42:23Z

@leventov do you have more comments on this pr?

leventov · 2019-07-26T17:19:03Z

+            return;
+          }
+        }
+        catch (Exception e) {


This is generally considered an antipattern to catch everything just in case unless the exception is rethrown in the catch block. Could catch only IOExceptions?

leventov · 2019-07-26T17:20:38Z

-  // Current total size of files in bytes. All accesses must be synchronized with files.
-  private long currSize = 0;
+  /**
+   * Current total size of files in bytes. All accesses must be synchronized with files.


"All accesses must be synchronized with files." sentence is strange. It seems to me that it should be just removed, GuardedBy already says everything needed.

leventov · 2019-07-26T17:21:03Z


-  // Set of files stored under the given path. All accesses must be synchronized with currSize.
+  /**
+   * Set of files stored under the given path. All accesses must be synchronized with currSizeBytes.


Same about "All accesses must be synchronized with currSizeBytes."

Could be under {@link #path}.

leventov · 2019-07-26T17:24:28Z

  private static final EmittingLogger log = new EmittingLogger(SegmentLoaderLocalCacheManager.class);
  private static final Comparator<StorageLocation> COMPARATOR = (left, right) ->
-      Longs.compare(right.available(), left.available());
+      Longs.compare(right.availableSizeBytes(), left.availableSizeBytes());


Comparator.comparingLong(StorageLocation::availableSizeBytes).reversed() would be clearer

leventov · 2019-07-26T17:47:09Z

-  private static final Comparator<StorageLocation> COMPARATOR = (left, right) ->
-      Longs.compare(right.availableSizeBytes(), left.availableSizeBytes());
+  private static final Comparator<StorageLocation> COMPARATOR = Comparator
+      .comparing(StorageLocation::availableSizeBytes)


Could be comparingLong?

Oh yeah. Fixed.

Fix race between canHandle() and addSegment() in StorageLocation

a21655a

jihoonson added the Bug label Jul 19, 2019

add comment

d75b8de

jihoonson mentioned this pull request Jul 19, 2019

Add shuffleSegmentPusher for data shuffle #8115

Merged

2 tasks

leventov self-requested a review July 20, 2019 07:51

add comments

d7903f5

fix test

5a1acaa

clintropolis reviewed Jul 23, 2019

View reviewed changes

leventov requested changes Jul 23, 2019

View reviewed changes

jihoonson added 2 commits July 23, 2019 16:24

address comments

a8e894f

remove <p> tag from javadoc

9ca05b6

clintropolis approved these changes Jul 24, 2019

View reviewed changes

leventov requested changes Jul 26, 2019

View reviewed changes

leventov reviewed Jul 26, 2019

View reviewed changes

address comments

f674682

leventov requested changes Jul 26, 2019

View reviewed changes

comparingLong

6532313

leventov merged commit adf7baf into apache:master Jul 27, 2019

clintropolis added this to the 0.16.0 milestone Aug 8, 2019

Conversation

jihoonson commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

leventov commented Jul 20, 2019

Uh oh!

jihoonson commented Jul 21, 2019

Uh oh!

clintropolis 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!

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!

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!

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!

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!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Jul 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jihoonson commented Jul 19, 2019 •

edited

Loading