You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by jl...@apache.org on 2023/02/08 06:48:01 UTC
[pinot] branch master updated: Fix race condition on removing aged deleted segments (#10238)
This is an automated email from the ASF dual-hosted git repository.
jlli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new f17c820041 Fix race condition on removing aged deleted segments (#10238)
f17c820041 is described below
commit f17c82004123315a5c0a5539e942934f38c96df9
Author: Jialiang Li <jl...@linkedin.com>
AuthorDate: Tue Feb 7 22:47:53 2023 -0800
Fix race condition on removing aged deleted segments (#10238)
Co-authored-by: Jack Li(Analytics Engineering) <jl...@jlli-mn1.linkedin.biz>
---
.../org/apache/pinot/common/utils/URIUtils.java | 13 +++++++
.../apache/pinot/common/utils/URIUtilsTest.java | 13 +++++++
.../helix/core/SegmentDeletionManager.java | 40 ++++++++++++----------
.../helix/core/retention/RetentionManager.java | 2 +-
.../helix/core/retention/RetentionManagerTest.java | 20 +++++------
.../core/util/SegmentDeletionManagerTest.java | 11 +++---
6 files changed, 65 insertions(+), 34 deletions(-)
diff --git a/pinot-common/src/main/java/org/apache/pinot/common/utils/URIUtils.java b/pinot-common/src/main/java/org/apache/pinot/common/utils/URIUtils.java
index 042427b772..5aff60b07f 100644
--- a/pinot-common/src/main/java/org/apache/pinot/common/utils/URIUtils.java
+++ b/pinot-common/src/main/java/org/apache/pinot/common/utils/URIUtils.java
@@ -70,6 +70,19 @@ public class URIUtils {
return stringJoiner.toString();
}
+ /**
+ * Returns the last part for the given path split by the file separator.
+ * If the file separator is not found, returns the whole path as the last part.
+ */
+ public static String getLastPart(String path) {
+ if (path == null) {
+ return null;
+ }
+ int parameterIndex = path.indexOf("?");
+ path = parameterIndex >= 0 ? path.substring(0, parameterIndex) : path;
+ return path.substring(path.lastIndexOf(File.separator) + 1);
+ }
+
/**
* Returns the download URL with the segment name encoded.
*/
diff --git a/pinot-common/src/test/java/org/apache/pinot/common/utils/URIUtilsTest.java b/pinot-common/src/test/java/org/apache/pinot/common/utils/URIUtilsTest.java
index 4cec446067..18022a9d5b 100644
--- a/pinot-common/src/test/java/org/apache/pinot/common/utils/URIUtilsTest.java
+++ b/pinot-common/src/test/java/org/apache/pinot/common/utils/URIUtilsTest.java
@@ -28,6 +28,7 @@ import org.testng.Assert;
import org.testng.annotations.Test;
import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
public class URIUtilsTest {
@@ -52,6 +53,18 @@ public class URIUtilsTest {
assertEquals(URIUtils.getPath("file:/foo/bar", "table", "segment+%25"), "file:/foo/bar/table/segment+%25");
}
+ @Test
+ public void testGetLastPart() {
+ assertNull(URIUtils.getLastPart(null));
+ assertEquals(URIUtils.getLastPart(""), "");
+ assertEquals(URIUtils.getLastPart("http://foo/bar"), "bar");
+ assertEquals(URIUtils.getLastPart("http://foo/bar?moo=x"), "bar");
+ assertEquals(URIUtils.getLastPart("?"), "");
+ assertEquals(URIUtils.getLastPart("?moo=x"), "");
+ assertEquals(URIUtils.getLastPart("/foo/bar"), "bar");
+ assertEquals(URIUtils.getLastPart("file:/foo/bar"), "bar");
+ }
+
@Test
public void testConstructDownloadUrl() {
assertEquals(URIUtils.constructDownloadUrl("http://foo/bar", "table", "segment"),
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
index ff74bea708..ecc7d337e8 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
@@ -44,6 +44,7 @@ import org.apache.helix.zookeeper.datamodel.ZNRecord;
import org.apache.pinot.common.metadata.ZKMetadataProvider;
import org.apache.pinot.common.utils.SegmentName;
import org.apache.pinot.common.utils.URIUtils;
+import org.apache.pinot.controller.LeadControllerManager;
import org.apache.pinot.spi.config.table.SegmentsValidationAndRetentionConfig;
import org.apache.pinot.spi.config.table.TableConfig;
import org.apache.pinot.spi.filesystem.PinotFS;
@@ -264,7 +265,7 @@ public class SegmentDeletionManager {
/**
* Removes aged deleted segments from the deleted directory
*/
- public void removeAgedDeletedSegments() {
+ public void removeAgedDeletedSegments(LeadControllerManager leadControllerManager) {
if (_dataDir != null) {
URI deletedDirURI = URIUtils.getUri(_dataDir, DELETED_SEGMENTS);
PinotFS pinotFS = PinotFSFactory.create(deletedDirURI.getScheme());
@@ -287,26 +288,29 @@ public class SegmentDeletionManager {
}
for (String tableNameDir : tableNameDirs) {
- URI tableNameURI = URIUtils.getUri(tableNameDir);
- // Get files that are aged
- final String[] targetFiles = pinotFS.listFiles(tableNameURI, false);
- int numFilesDeleted = 0;
- for (String targetFile : targetFiles) {
- URI targetURI = URIUtils.getUri(targetFile);
- long deletionTimeMs = getDeletionTimeMsFromFile(targetFile, pinotFS.lastModified(targetURI));
- if (System.currentTimeMillis() >= deletionTimeMs) {
- if (!pinotFS.delete(targetURI, true)) {
- LOGGER.warn("Cannot remove file {} from deleted directory.", targetURI.toString());
- } else {
- numFilesDeleted++;
+ String tableName = URIUtils.getLastPart(tableNameDir);
+ if (leadControllerManager.isLeaderForTable(tableName)) {
+ URI tableNameURI = URIUtils.getUri(tableNameDir);
+ // Get files that are aged
+ final String[] targetFiles = pinotFS.listFiles(tableNameURI, false);
+ int numFilesDeleted = 0;
+ for (String targetFile : targetFiles) {
+ URI targetURI = URIUtils.getUri(targetFile);
+ long deletionTimeMs = getDeletionTimeMsFromFile(targetFile, pinotFS.lastModified(targetURI));
+ if (System.currentTimeMillis() >= deletionTimeMs) {
+ if (!pinotFS.delete(targetURI, true)) {
+ LOGGER.warn("Cannot remove file {} from deleted directory.", targetURI);
+ } else {
+ numFilesDeleted++;
+ }
}
}
- }
- if (numFilesDeleted == targetFiles.length) {
- // Delete directory if it's empty
- if (!pinotFS.delete(tableNameURI, false)) {
- LOGGER.warn("The directory {} cannot be removed.", tableNameDir);
+ if (numFilesDeleted == targetFiles.length) {
+ // Delete directory if it's empty
+ if (!pinotFS.delete(tableNameURI, false)) {
+ LOGGER.warn("The directory {} cannot be removed.", tableNameDir);
+ }
}
}
}
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
index 624ee914d7..ea9cda626f 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
@@ -94,7 +94,7 @@ public class RetentionManager extends ControllerPeriodicTask<Void> {
@Override
protected void postprocess() {
LOGGER.info("Removing aged deleted segments for all tables");
- _pinotHelixResourceManager.getSegmentDeletionManager().removeAgedDeletedSegments();
+ _pinotHelixResourceManager.getSegmentDeletionManager().removeAgedDeletedSegments(_leadControllerManager);
}
private void manageRetentionForTable(TableConfig tableConfig) {
diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java
index 85aba90f36..6da50f5c5b 100644
--- a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java
+++ b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java
@@ -77,11 +77,10 @@ public class RetentionManagerTest {
segmentsZKMetadata.add(segmentZKMetadata);
}
final TableConfig tableConfig = createOfflineTableConfig();
- PinotHelixResourceManager pinotHelixResourceManager = mock(PinotHelixResourceManager.class);
- setupPinotHelixResourceManager(tableConfig, removedSegments, pinotHelixResourceManager);
-
LeadControllerManager leadControllerManager = mock(LeadControllerManager.class);
when(leadControllerManager.isLeaderForTable(anyString())).thenReturn(true);
+ PinotHelixResourceManager pinotHelixResourceManager = mock(PinotHelixResourceManager.class);
+ setupPinotHelixResourceManager(tableConfig, removedSegments, pinotHelixResourceManager, leadControllerManager);
when(pinotHelixResourceManager.getTableConfig(OFFLINE_TABLE_NAME)).thenReturn(tableConfig);
when(pinotHelixResourceManager.getSegmentsZKMetadata(OFFLINE_TABLE_NAME)).thenReturn(segmentsZKMetadata);
@@ -98,7 +97,7 @@ public class RetentionManagerTest {
SegmentDeletionManager deletionManager = pinotHelixResourceManager.getSegmentDeletionManager();
// Verify that the removeAgedDeletedSegments() method in deletion manager is actually called.
- verify(deletionManager, times(1)).removeAgedDeletedSegments();
+ verify(deletionManager, times(1)).removeAgedDeletedSegments(leadControllerManager);
// Verify that the deleteSegments method is actually called.
verify(pinotHelixResourceManager, times(1)).deleteSegments(anyString(), anyList());
@@ -161,7 +160,7 @@ public class RetentionManagerTest {
}
private void setupPinotHelixResourceManager(TableConfig tableConfig, final List<String> removedSegments,
- PinotHelixResourceManager resourceManager) {
+ PinotHelixResourceManager resourceManager, LeadControllerManager leadControllerManager) {
final String tableNameWithType = tableConfig.getTableName();
when(resourceManager.getAllTables()).thenReturn(Collections.singletonList(tableNameWithType));
@@ -177,7 +176,7 @@ public class RetentionManagerTest {
throws Throwable {
return null;
}
- }).when(deletionManager).removeAgedDeletedSegments();
+ }).when(deletionManager).removeAgedDeletedSegments(leadControllerManager);
when(resourceManager.getSegmentDeletionManager()).thenReturn(deletionManager);
// If and when PinotHelixResourceManager.deleteSegments() is invoked, make sure that the segments deleted
@@ -210,12 +209,11 @@ public class RetentionManagerTest {
TableConfig tableConfig = createRealtimeTableConfig1(replicaCount);
List<String> removedSegments = new ArrayList<>();
- PinotHelixResourceManager pinotHelixResourceManager =
- setupSegmentMetadata(tableConfig, now, initialNumSegments, removedSegments);
- setupPinotHelixResourceManager(tableConfig, removedSegments, pinotHelixResourceManager);
-
LeadControllerManager leadControllerManager = mock(LeadControllerManager.class);
when(leadControllerManager.isLeaderForTable(anyString())).thenReturn(true);
+ PinotHelixResourceManager pinotHelixResourceManager =
+ setupSegmentMetadata(tableConfig, now, initialNumSegments, removedSegments);
+ setupPinotHelixResourceManager(tableConfig, removedSegments, pinotHelixResourceManager, leadControllerManager);
ControllerConf conf = new ControllerConf();
ControllerMetrics controllerMetrics = new ControllerMetrics(PinotMetricUtils.getPinotMetricsRegistry());
@@ -229,7 +227,7 @@ public class RetentionManagerTest {
SegmentDeletionManager deletionManager = pinotHelixResourceManager.getSegmentDeletionManager();
// Verify that the removeAgedDeletedSegments() method in deletion manager is actually called.
- verify(deletionManager, times(1)).removeAgedDeletedSegments();
+ verify(deletionManager, times(1)).removeAgedDeletedSegments(leadControllerManager);
// Verify that the deleteSegments method is actually called.
verify(pinotHelixResourceManager, times(1)).deleteSegments(anyString(), anyList());
diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/util/SegmentDeletionManagerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/util/SegmentDeletionManagerTest.java
index a617dd8612..b4d0356c5a 100644
--- a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/util/SegmentDeletionManagerTest.java
+++ b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/util/SegmentDeletionManagerTest.java
@@ -38,6 +38,7 @@ import org.apache.helix.model.ExternalView;
import org.apache.helix.model.IdealState;
import org.apache.helix.store.zk.ZkHelixPropertyStore;
import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.controller.LeadControllerManager;
import org.apache.pinot.controller.helix.core.SegmentDeletionManager;
import org.apache.pinot.spi.config.table.SegmentsValidationAndRetentionConfig;
import org.apache.pinot.spi.config.table.TableConfig;
@@ -228,9 +229,11 @@ public class SegmentDeletionManagerTest {
tempDir.deleteOnExit();
FakeDeletionManager deletionManager = new FakeDeletionManager(
tempDir.getAbsolutePath(), helixAdmin, propertyStore, 7);
+ LeadControllerManager leadControllerManager = mock(LeadControllerManager.class);
+ when(leadControllerManager.isLeaderForTable(anyString())).thenReturn(true);
// Test delete when deleted segments directory does not exists
- deletionManager.removeAgedDeletedSegments();
+ deletionManager.removeAgedDeletedSegments(leadControllerManager);
// Create deleted directory
String deletedDirectoryPath = tempDir + File.separator + "Deleted_Segments";
@@ -238,7 +241,7 @@ public class SegmentDeletionManagerTest {
deletedDirectory.mkdir();
// Test delete when deleted segments directory is empty
- deletionManager.removeAgedDeletedSegments();
+ deletionManager.removeAgedDeletedSegments(leadControllerManager);
// Create dummy directories and files
File dummyDir1 = new File(deletedDirectoryPath + File.separator + "dummy1");
@@ -249,7 +252,7 @@ public class SegmentDeletionManagerTest {
dummyDir3.mkdir();
// Test delete when there is no files but some directories exist
- deletionManager.removeAgedDeletedSegments();
+ deletionManager.removeAgedDeletedSegments(leadControllerManager);
Assert.assertEquals(dummyDir1.exists(), false);
Assert.assertEquals(dummyDir2.exists(), false);
Assert.assertEquals(dummyDir3.exists(), false);
@@ -279,7 +282,7 @@ public class SegmentDeletionManagerTest {
Assert.assertEquals(dummyDir3.list().length, 3);
// Try to remove files with the retention of 1 days.
- deletionManager.removeAgedDeletedSegments();
+ deletionManager.removeAgedDeletedSegments(leadControllerManager);
// Check that only 1 day retention file is remaining
Assert.assertEquals(dummyDir1.list().length, 1);
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org