Refactoring DiskSpaceAllocator to support dynamically add/remove store #1210

jsjtzyy · 2019-07-06T18:02:00Z

This PR allows disk allocator to add or delete segments for a certain
store without re-initializing entire reserve pool. The changes include:

Each store has its own reserve file directory.
All stores on the same disk share reserved swap segments.
Changed addRequiredSegments() and deleteExtraSegments() to support
segments addition/removal.

codecov-io · 2019-07-06T18:18:45Z

Codecov Report

Merging #1210 into master will increase coverage by 0.05%.
The diff coverage is 87.42%.

@@             Coverage Diff              @@
##             master    #1210      +/-   ##
============================================
+ Coverage     69.49%   69.54%   +0.05%     
- Complexity     5516     5551      +35     
============================================
  Files           432      432              
  Lines         33844    33943      +99     
  Branches       4312     4342      +30     
============================================
+ Hits          23520    23606      +86     
- Misses         9137     9140       +3     
- Partials       1187     1197      +10

Impacted Files	Coverage Δ	Complexity Δ
...ava/com.github.ambry.store/BlobStoreCompactor.java	`91.97% <ø> (ø)`	`154 <0> (ø)`	⬇️
...tore/src/main/java/com.github.ambry.store/Log.java	`98.55% <100%> (+0.02%)`	`60 <0> (+1)`	⬆️
.../com.github.ambry.store/DiskSpaceRequirements.java	`100% <100%> (+9.09%)`	`9 <5> (+2)`	⬆️
...rc/main/java/com.github.ambry.store/BlobStore.java	`88.3% <100%> (+0.02%)`	`89 <0> (ø)`	⬇️
...ava/com.github.ambry.store/DiskSpaceAllocator.java	`89.84% <85.61%> (-3.33%)`	`68 <42> (+30)`
.../main/java/com.github.ambry.store/ScanResults.java	`81.25% <0%> (-1.57%)`	`16% <0%> (-1%)`
.../src/main/java/com.github.ambry.store/Journal.java	`90.76% <0%> (-1.54%)`	`24% <0%> (-1%)`
...in/java/com.github.ambry.store/BlobStoreStats.java	`71.34% <0%> (-1.24%)`	`103% <0%> (-2%)`
...va/com.github.ambry.router/TtlUpdateOperation.java	`86.52% <0%> (-1.15%)`	`48% <0%> (-3%)`
...src/main/java/com.github.ambry.commons/BlobId.java	`93.18% <0%> (-0.36%)`	`71% <0%> (-1%)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80c32e7...2664d0d. Read the comment docs.

jsjtzyy · 2019-07-09T21:48:02Z

./gradlew build && ./gradlew test succeeded
The new hierarchy of reserve pool is as follows:

   * --- reserve_pool
   *       |--- reserve_store_1
   *       |              |--- reserve_size_2147483648
   *       |
   *       |--- reserve_store_2
   *                      |--- reserve_size_2147483648
   *       ...
   *       |--- reserve_store_n
   *       |              |--- reserve_size_2147483648
   *       |
   *       |--- reserve_swap
   *                      |--- reserve_size_2147483648
   *                      |--- reserve_size_8589934592

cgtz · 2019-07-17T23:07:07Z

ambry-store/src/main/java/com.github.ambry.store/BlobStoreCompactor.java

@@ -256,6 +256,7 @@ A single compaction job could be performed across multiple compaction cycles (if
   * @return the number of temporary log segment files this compactor is currently using.
   */
  int getSwapSegmentsInUse() throws StoreException {
+    // TODO: use this method to return swap segment to pool when removing store


address this TODO?

This TODO is just a reminder that future PR (dynamically remove store) could invoke this method to swap segment in use and return it to pool. For current PR, no extra changes are needed. I will remove this comment in "remove store" PR :)

cgtz · 2019-07-17T23:30:59Z

ambry-store/src/main/java/com.github.ambry.store/DiskSpaceAllocator.java

-  private static final FileFilter RESERVE_FILE_FILTER =
+  private static final String RESERVE_FILE_PREFIX = "reserve_";
+  static final String STORE_DIR_PREFIX = "reserve_store_";
+  static final FileFilter RESERVE_FILE_FILTER =


the two filters seem like they can be private

cgtz · 2019-07-17T23:49:31Z

ambry-store/src/main/java/com.github.ambry.store/DiskSpaceAllocator.java

   */
  DiskSpaceAllocator(boolean enablePooling, File reserveDir, long requiredSwapSegmentsPerSize,
      StorageManagerMetrics metrics) {
    this.enablePooling = enablePooling;
    this.reserveDir = reserveDir;
+    swapReserveDir = new File(reserveDir, SWAP_DIR_NAME);


What is the rollout strategy for this? Is it better to manually delete the old directories or have the code detect the old format and do it automatically. We could automatically delete any folders in reserveDir that start with "reserve_size_". This could help to avoid a complicated deployment where we have to disable the diskspaceallocator, then delete the files, and then redeploy with the new version.

True. I will add some logic to correctly identify stale files/directories and delete them automatically.

Actually, after second look, my code will clean up the directories that are neither store reserve directory nor swap segment directory. (following piece of code is in inventoryExistingReserveFiles method)

} else { // if it is neither store reserved segment directory nor swap segment directory, then delete it. if (!file.delete()) { throw new IOException("Could not delete the following reserve file or directory: " + file.getAbsolutePath()); } }

I will ensure there is a test to verify this.

cgtz · 2019-07-18T00:16:51Z

ambry-store/src/main/java/com.github.ambry.store/DiskSpaceAllocator.java

+          // attempt to get segment from corresponding store reserve directory
+          if (storeReserveFiles.containsKey(storeId)) {
+            reserveFile = storeReserveFiles.get(storeId).remove(sizeInBytes);
+          }


add logging if storeId not found?

cgtz · 2019-07-18T00:17:47Z

ambry-store/src/main/java/com.github.ambry.store/DiskSpaceAllocator.java

+          if (storeReserveFiles.containsKey(storeId)) {
+            reserveFile = storeReserveFiles.get(storeId).remove(sizeInBytes);
+          }
+        }
      }
      if (reserveFile == null) {
        if (enablePooling) {


or it might be good to edit all these log message to include info about storeID and isSwapSegment

good point, will do.

jsjtzyy · 2019-07-22T20:48:26Z

added unit test to verify inventory can delete invalid directories/files.
./gradlew build && ./gradlew test succeeded

zzmao

PROD code looks good. I can merge it now since this change blocks @jsjtzyy .

This PR allows disk allocator to add or delete segments for a certain store without re-initializing entire reserve pool. The changes include: 1. Each store has its own reserve file directory. 2. All stores on the same disk share reserved swap segments. 3. Changed addRequiredSegments() and deleteExtraSegments() to support segments addition/removal.

cgtz · 2019-07-29T21:17:30Z

ambry-store/src/main/java/com.github.ambry.store/DiskSpaceAllocator.java

      }
    } finally {
      long elapsedTime = System.currentTimeMillis() - startTime;
-      logger.debug("free took {} ms", elapsedTime);
+      logger.debug("free took {} ms for {}", elapsedTime, "store[" + storeId + "]");


to avoid the cost of string concat when debug logging is off:

logger.debug("free took {} ms for store[{}]", elapsedTime, storeId);

jsjtzyy force-pushed the disk-allocator branch 2 times, most recently from 25f0c83 to f7ffab5 Compare July 6, 2019 22:49

jsjtzyy self-assigned this Jul 6, 2019

jsjtzyy marked this pull request as ready for review July 8, 2019 16:29

jsjtzyy requested review from cgtz and zzmao July 8, 2019 22:10

jsjtzyy force-pushed the disk-allocator branch from f7ffab5 to 1690176 Compare July 11, 2019 20:12

cgtz reviewed Jul 18, 2019

View reviewed changes

jsjtzyy force-pushed the disk-allocator branch from 1690176 to 8296bbd Compare July 22, 2019 16:18

zzmao approved these changes Jul 22, 2019

View reviewed changes

jsjtzyy added 2 commits July 26, 2019 10:17

added unit tests and log infos

2372da7

jsjtzyy force-pushed the disk-allocator branch from 8296bbd to 2372da7 Compare July 26, 2019 17:19

jsjtzyy mentioned this pull request Jul 29, 2019

Support dynamically adding store to storage manager #1226

Merged

cgtz approved these changes Jul 30, 2019

View reviewed changes

address comment

2664d0d

cgtz merged commit f9f246f into linkedin:master Jul 30, 2019

jsjtzyy mentioned this pull request Aug 15, 2019

DiskSpaceAllocatorPool is currently not thread-safe for runtime initialization #1061

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring DiskSpaceAllocator to support dynamically add/remove store #1210

Refactoring DiskSpaceAllocator to support dynamically add/remove store #1210

jsjtzyy commented Jul 6, 2019 •

edited

Loading

codecov-io commented Jul 6, 2019 •

edited

Loading

jsjtzyy commented Jul 9, 2019

cgtz Jul 17, 2019

jsjtzyy Jul 19, 2019 •

edited

Loading

cgtz Jul 17, 2019

jsjtzyy Jul 20, 2019

cgtz Jul 17, 2019

jsjtzyy Jul 19, 2019

jsjtzyy Jul 20, 2019 •

edited

Loading

cgtz Jul 18, 2019

cgtz Jul 18, 2019

jsjtzyy Jul 19, 2019

jsjtzyy commented Jul 22, 2019

zzmao left a comment

cgtz Jul 29, 2019

Refactoring DiskSpaceAllocator to support dynamically add/remove store #1210

Refactoring DiskSpaceAllocator to support dynamically add/remove store #1210

Conversation

jsjtzyy commented Jul 6, 2019 • edited Loading

codecov-io commented Jul 6, 2019 • edited Loading

Codecov Report

jsjtzyy commented Jul 9, 2019

Choose a reason for hiding this comment

jsjtzyy Jul 19, 2019 • edited Loading

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

jsjtzyy Jul 20, 2019 • edited Loading

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

jsjtzyy commented Jul 22, 2019

zzmao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsjtzyy commented Jul 6, 2019 •

edited

Loading

codecov-io commented Jul 6, 2019 •

edited

Loading

jsjtzyy Jul 19, 2019 •

edited

Loading

jsjtzyy Jul 20, 2019 •

edited

Loading