This is an automated email from the ASF dual-hosted git repository.
kharekartik 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 9c3a16be74d Fix retry of partial table deletion (#16835)
9c3a16be74d is described below
commit 9c3a16be74d3f4ed150b1de9acb289c0edbcaece
Author: Shounak kulkarni <[email protected]>
AuthorDate: Wed Sep 17 19:25:50 2025 +0530
Fix retry of partial table deletion (#16835)
* Bubble up meaningful exception for missing IS during table update
* Update table config only when required. Ignore table update failures.
* Add test to ensure table deletion retry goes through
---
.../api/resources/PinotTableRestletResource.java | 13 ++++++--
.../helix/core/PinotHelixResourceManager.java | 4 +++
.../api/PinotTableRestletResourceTest.java | 36 ++++++++++++++++++++++
.../utils/builder/ControllerRequestURLBuilder.java | 4 +++
4 files changed, 55 insertions(+), 2 deletions(-)
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
index 898fd26e7b0..a27d5ce3853 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
@@ -509,11 +509,20 @@ public class PinotTableRestletResource {
}
Map<String, Map<String, String>> taskTypeConfigsMap =
tableConfig.getTaskConfig().getTaskTypeConfigsMap();
Set<String> taskTypes = taskTypeConfigsMap.keySet();
+ boolean tableConfigChanged = false;
for (String taskType : taskTypes) {
// remove the task schedules to avoid task being scheduled during table
deletion
- taskTypeConfigsMap.get(taskType).remove(PinotTaskManager.SCHEDULE_KEY);
+ tableConfigChanged =
+ tableConfigChanged ||
taskTypeConfigsMap.get(taskType).remove(PinotTaskManager.SCHEDULE_KEY) != null;
+ }
+ if (tableConfigChanged) {
+ try {
+ pinotHelixResourceManager.updateTableConfig(tableConfig);
+ } catch (Exception e) {
+ LOGGER.warn("Unable to remove the task schedules, going ahead with
table deletion anyways. "
+ + "Reason for failure : {}", e.getMessage());
+ }
}
- pinotHelixResourceManager.updateTableConfig(tableConfig);
List<String> pendingTasks = new ArrayList<>();
for (String taskType : taskTypes) {
Map<String, TaskState> taskStates;
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
index 97e2cb0a908..fe20b4359de 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
@@ -2187,6 +2187,10 @@ public class PinotHelixResourceManager {
// Update IdealState replication
IdealState idealState =
_helixAdmin.getResourceIdealState(_helixClusterName, tableNameWithType);
+ Preconditions.checkArgument(idealState != null,
+ "Ideal state is not present for the table " + tableNameWithType + ". "
+ + "Its possible due to ongoing/incomplete table deletion. "
+ + "Please re-trigger the table delete operation to clean it up and
recreate the table");
String replicationConfigured =
Integer.toString(tableConfig.getReplication());
if (!idealState.getReplicas().equals(replicationConfigured)) {
HelixHelper.updateIdealState(_helixZkManager, tableNameWithType, is -> {
diff --git
a/pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java
b/pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java
index 7af7ad37345..f0261829338 100644
---
a/pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java
+++
b/pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java
@@ -24,6 +24,8 @@ import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import java.io.IOException;
+import java.net.URLEncoder;
+import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
@@ -63,6 +65,7 @@ import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
import org.apache.pinot.util.TestUtils;
import org.mockito.Mockito;
+import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.AfterMethod;
import org.testng.annotations.BeforeClass;
@@ -1052,6 +1055,39 @@ public class PinotTableRestletResourceTest extends
ControllerTest {
InstanceAssignmentConfig.PartitionSelector.FD_AWARE_INSTANCE_PARTITION_SELECTOR.name(),
false);
}
+ @Test
+ public void testTableDeletionFromPreviousIncompleteDeletion()
+ throws Exception {
+ String tableName = "testTableDeletionValidation";
+ DEFAULT_INSTANCE.addDummySchema(tableName);
+
+ TableConfig offlineTableConfig = getOfflineTableBuilder(tableName)
+ .setTaskConfig(new TableTaskConfig(ImmutableMap.of(
+ MinionConstants.SegmentGenerationAndPushTask.TASK_TYPE,
Map.of("schedule", "0 0 * * * ? *"))))
+ .build();
+
+ String tableNameWithType = offlineTableConfig.getTableName();
+ String creationResponse = sendPostRequest(_createTableUrl,
offlineTableConfig.toJsonString());
+ assertEquals(creationResponse,
+ "{\"unrecognizedProperties\":{},\"status\":\"Table " +
tableNameWithType + " successfully added\"}");
+
+ String encodedISPath =
+ URLEncoder.encode("/ControllerTest/IDEALSTATES/" + tableNameWithType,
Charset.defaultCharset());
+
sendDeleteRequest(DEFAULT_INSTANCE.getControllerRequestURLBuilder().forZkDelete(encodedISPath));
+ // Table deletion will throw exception but internally it should clean up
all the dangling table resources
+ Assert.expectThrows(IOException.class,
+ () ->
sendDeleteRequest(DEFAULT_INSTANCE.getControllerRequestURLBuilder().forTableDelete(tableName)));
+
+ String encodedTableConfigPath =
URLEncoder.encode("/ControllerTest/PROPERTYSTORE/CONFIGS/TABLE/"
+ + tableNameWithType, Charset.defaultCharset());
+ try {
+
sendGetRequest(DEFAULT_INSTANCE.getControllerRequestURLBuilder().forZkGet(encodedTableConfigPath));
+ fail("Table config node should be deleted so get request should fail");
+ } catch (IOException e) {
+ assertTrue(e.getMessage().contains(tableNameWithType + " does not
exist"));
+ }
+ }
+
@Test
public void testTableTasksValidationWithNoDanglingTasks()
throws Exception {
diff --git
a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java
b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java
index 1413a367954..693de77733f 100644
---
a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java
+++
b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java
@@ -652,6 +652,10 @@ public class ControllerRequestURLBuilder {
return StringUtil.join("/", _baseUrl, "zk/delete");
}
+ public String forZkDelete(String path) {
+ return StringUtil.join("/", _baseUrl, "zk/delete", "?path=" + path);
+ }
+
public String forZkGet(String path) {
return StringUtil.join("/", _baseUrl, "zk/get", "?path=" + path);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
