Support dynamically removing store from storage manager #1232

jsjtzyy · 2019-08-08T05:52:38Z

Given partition id, remove the corresponding store from storage
manager, disk manager and compaction manager.
Support deleting allocated files of store and returning swap
segments to reserver pool if necessary.

codecov-io · 2019-08-08T16:58:18Z

Codecov Report

Merging #1232 into master will decrease coverage by 16.7%.
The diff coverage is 95.71%.

@@              Coverage Diff              @@
##             master    #1232       +/-   ##
=============================================
- Coverage     88.92%   72.21%   -16.71%     
- Complexity       60     6054     +5994     
=============================================
  Files             6      439      +433     
  Lines           352    34968    +34616     
  Branches         37     4437     +4400     
=============================================
+ Hits            313    25252    +24939     
- Misses           29     8560     +8531     
- Partials         10     1156     +1146

Impacted Files	Coverage Δ	Complexity Δ
.../com.github.ambry.store/StorageManagerMetrics.java	`98.07% <100%> (ø)`	`6 <1> (?)`
...in/java/com.github.ambry.store/StorageManager.java	`88.73% <100%> (ø)`	`47 <3> (?)`
...rc/main/java/com.github.ambry.store/BlobStore.java	`90.42% <100%> (ø)`	`97 <3> (?)`
...main/java/com.github.ambry.store/StoreMetrics.java	`97.2% <100%> (ø)`	`10 <6> (?)`
...java/com.github.ambry.store/CompactionManager.java	`89.28% <100%> (ø)`	`25 <3> (?)`
...ava/com.github.ambry.store/BlobStoreCompactor.java	`92.12% <100%> (ø)`	`156 <0> (?)`
.../main/java/com.github.ambry.store/DiskManager.java	`86.6% <81.25%> (ø)`	`55 <4> (?)`
.../com/github/ambry/account/HelixAccountService.java	`87.05% <0%> (-0.94%)`	`33% <0%> (-5%)`
...om/github/ambry/account/AccountServiceMetrics.java	`100% <0%> (ø)`	`1% <0%> (ø)`	⬇️
...ava/com.github.ambry/config/PerformanceConfig.java	`100% <0%> (ø)`	`4% <0%> (?)`
... and 432 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 e5d6e7e...8ea2d7a. Read the comment docs.

jsjtzyy · 2019-08-12T20:07:46Z

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

+  boolean removeBlobStore(BlobStore store) {
+    boolean result;
+    if (compactionExecutor == null) {
+      stores.remove(store);


Once #1226 is merged, stores will become concurrent set which should be ok here.

1. Given partition id, remove the corresponding store from storage manager, disk manager and compaction manager. 2. Support deleting allocated files of store and returning swap segments to reserver pool if necessary.

cgtz

LGTM after comments addressed

cgtz · 2019-08-15T20:51:13Z

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

+    String[] swapSegmentsInUse = compactor.getSwapSegmentsInUse();
+    if (swapSegmentsInUse.length > 0) {
+      for (String fileName : swapSegmentsInUse) {
+        logger.trace("Returning swap segment {} to reserve pool", fileName);


Maybe log this at info level since it is relatively rare

cgtz · 2019-08-15T20:51:39Z

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

+    }
+    // step1: return occupied swap segments (if any) to reserve pool
+    String[] swapSegmentsInUse = compactor.getSwapSegmentsInUse();
+    if (swapSegmentsInUse.length > 0) {


don't need this if around the for loop

cgtz · 2019-08-15T21:13:06Z

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

+    logger.info("Deleting store {} directory", storeId);
+    File storeDir = new File(dataDir);
+    try {
+      Files.walk(storeDir.toPath()).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);


We also have Utils.deleteFileOrDirectory that you can use

ah, I didn't notice that. Will change this piece of code.

cgtz · 2019-08-15T21:17:02Z

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

+    if (compactionExecutor == null) {
+      stores.remove(store);
+      result = true;
+    } else {


I think you can remove this nested else and use else if (!compact...

cgtz · 2019-08-15T21:36:03Z

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

+   * @param id the {@link PartitionId} associated with store
+   * @return {@code true} if removal succeeds. {@code false} otherwise.
+   */
+  public boolean removeBlobStore(PartitionId id) {


Any reason for this method (and the ones in DiskManager and CompactionManager) to return success/failure booleans instead of throwing exceptions? With exceptions, you can log just once at the top level

I am slightly leaning towards current way to make it sort of consistent with scheduleNextForCompaction, controlCompactionForBlobStore etc. Returning boolean can be explicitly verified in unit tests.

cgtz · 2019-08-15T21:37:27Z

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

@@ -219,6 +219,8 @@ private void deregisterIndexGauges(String storeId) {
    registry.remove(MetricRegistry.name(Log.class, prefix + "CurrentCapacityUsed"));
    registry.remove(MetricRegistry.name(Log.class, prefix + "PercentageUsedCapacity"));
    registry.remove(MetricRegistry.name(Log.class, prefix + "CurrentSegmentCount"));
+    registry.remove(MetricRegistry.name(Log.class, "ByteBufferForAppendTotalCount"));
+    registry.remove(MetricRegistry.name(Log.class, "UnderCompaction" + SEPERATOR + "ByteBufferForAppendTotalCount"));


minor typo: SEPARATOR

cgtz · 2019-08-15T22:52:40Z

ambry-store/src/test/java/com.github.ambry.store/BlobStoreTest.java

+   */
+  @Test
+  public void deleteStoreFilesTest() throws Exception {
+    assumeTrue(isLogSegmented);


Could this code be tested with non segmented logs? Perhaps just put the test of swap segments within if (isLogSegmented)

Good point. I added some codes to ensure both segmented and non-segmented cases are tests.

cgtz · 2019-08-15T22:53:15Z

ambry-store/src/test/java/com.github.ambry.store/BlobStoreTest.java

+    StoreConfig config = new StoreConfig(new VerifiableProperties(properties));
+    MetricRegistry registry = new MetricRegistry();
+    StoreMetrics metrics = new StoreMetrics(registry);
+    //createBlobStore(getMockReplicaId(storeDir.getAbsolutePath()));


remove commented code

jsjtzyy · 2019-08-19T17:42:46Z

Addressed @cgtz 's comments. @lightningrob gentle reminder to review.

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

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

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

lightningrob · 2019-08-19T17:55:35Z

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

+      stores.remove(store);
+      result = true;
+    } else if (!compactionExecutor.getStoresDisabledCompaction().contains(store)) {
+      logger.error("Fail to remove store ({}) from compaction manager because compaction of it is still enabled",


This seems like it should be an IllegalStateException. Is there a valid scenario where compaction is still enabled when removeStore is called? If we return false and compaction is turned off later, will the store ever be removed?

I agree with your point, however, I am trying to use same way in scheduleNextForCompaction method (line 360). Returning false may happen when disabling compaction on the store hasn't completed or hasn't been executed. The Helix state model will retry entire workflow (disable compaction, remove store) to guarantee the store is correctly removed.

cgtz · 2019-08-19T22:51:57Z

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

+    logger.info("Deleting store {} directory", storeId);
+    File storeDir = new File(dataDir);
+    try {
+      Utils.deleteFileOrDirectory(storeDir);


This method already throws IOException, so you probably don't have to catch the exception, unless you want dataDir to be in the message.

Also, I would recommend setting the cause exception to e if you keep the catch

Thanks for reminding me of this. I prefer to keep this catch in case there is any non-IOException. Also, I would take your advice to set the exception e

jsjtzyy self-assigned this Aug 8, 2019

jsjtzyy force-pushed the remove-store branch from 1bb103e to f86a962 Compare August 8, 2019 16:39

jsjtzyy marked this pull request as ready for review August 12, 2019 18:20

jsjtzyy requested review from cgtz and lightningrob August 12, 2019 18:20

jsjtzyy commented Aug 12, 2019

View reviewed changes

jsjtzyy force-pushed the remove-store branch from 8acb924 to 81684a1 Compare August 12, 2019 22:51

jsjtzyy added 4 commits August 15, 2019 14:24

Support dynamically removing store from storage manager

626c1fd

1. Given partition id, remove the corresponding store from storage manager, disk manager and compaction manager. 2. Support deleting allocated files of store and returning swap segments to reserver pool if necessary.

add unit tests

f14a130

add java docs

3f2b42c

minor fixes and increase test coverage

5bb12c7

jsjtzyy force-pushed the remove-store branch from 81684a1 to 5bb12c7 Compare August 15, 2019 21:26

cgtz suggested changes Aug 15, 2019

View reviewed changes

addressed comments

e167239

lightningrob reviewed Aug 19, 2019

View reviewed changes

partially addressed comments

b1f2bd0

cgtz approved these changes Aug 19, 2019

View reviewed changes

address comments

8ea2d7a

lightningrob approved these changes Aug 20, 2019

View reviewed changes

lightningrob merged commit 90e1227 into linkedin:master Aug 20, 2019

jsjtzyy mentioned this pull request Sep 16, 2019

Support adding BlobStore to StorageManager #924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support dynamically removing store from storage manager #1232

Support dynamically removing store from storage manager #1232

jsjtzyy commented Aug 8, 2019

codecov-io commented Aug 8, 2019 •

edited

Loading

jsjtzyy Aug 12, 2019

cgtz left a comment

cgtz Aug 15, 2019

cgtz Aug 15, 2019

cgtz Aug 15, 2019

jsjtzyy Aug 16, 2019

cgtz Aug 15, 2019

cgtz Aug 15, 2019

jsjtzyy Aug 16, 2019

cgtz Aug 15, 2019

jsjtzyy Aug 16, 2019

cgtz Aug 15, 2019

jsjtzyy Aug 16, 2019

cgtz Aug 15, 2019

jsjtzyy commented Aug 19, 2019

lightningrob Aug 19, 2019

jsjtzyy Aug 19, 2019

cgtz Aug 19, 2019

jsjtzyy Aug 19, 2019

Support dynamically removing store from storage manager #1232

Support dynamically removing store from storage manager #1232

Conversation

jsjtzyy commented Aug 8, 2019

codecov-io commented Aug 8, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

cgtz left a 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

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

jsjtzyy commented Aug 19, 2019

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

codecov-io commented Aug 8, 2019 •

edited

Loading