singhpk234 commented on code in PR #17691:
URL: https://github.com/apache/iceberg/pull/17691#discussion_r3800222605
##########
core/src/main/java/org/apache/iceberg/rest/InMemoryPlanningState.java:
##########
@@ -127,6 +127,41 @@ List<String> nextPlanTask(String planTaskKey) {
return ImmutableList.of();
}
+ /**
+ * Releases the state for a single fetched plan task. Called after a
successful fetch to prevent
+ * unbounded memory growth — the {@code FileScanTask} list is the dominant
memory consumer and
+ * need not be retained once the client has received it (#17427).
Review Comment:
do we need to add this in java doc ?
##########
core/src/main/java/org/apache/iceberg/rest/InMemoryPlanningState.java:
##########
@@ -127,6 +127,41 @@ List<String> nextPlanTask(String planTaskKey) {
return ImmutableList.of();
}
+ /**
+ * Releases the state for a single fetched plan task. Called after a
successful fetch to prevent
+ * unbounded memory growth — the {@code FileScanTask} list is the dominant
memory consumer and
+ * need not be retained once the client has received it (#17427).
+ *
+ * @param planTaskKey the plan task key to release
+ */
+ void releasePlanTask(String planTaskKey) {
+ planTaskToFileScanTasks.remove(planTaskKey);
+ planTaskToNext.remove(planTaskKey);
+ }
+
+ /**
+ * Releases the async planning state for the plan that owns the given plan
task key. Called when
+ * the last plan task in a chain is fetched, so completed plans don't
accumulate indefinitely
+ * (#17427).
+ *
+ * <p>The plan task key format is {@code {planId}-{tableId}-{sequence}}. The
planId is extracted by
+ * stripping the last two hyphen-separated components.
+ *
+ * @param planTaskKey a plan task key belonging to the plan whose async
state should be released
+ */
+ void releaseAsyncPlanForTask(String planTaskKey) {
+ int lastHyphen = planTaskKey.lastIndexOf('-');
+ if (lastHyphen < 0) {
+ return;
+ }
+ int secondLastHyphen = planTaskKey.lastIndexOf('-', lastHyphen - 1);
+ if (secondLastHyphen < 0) {
+ return;
+ }
+ String planId = planTaskKey.substring(0, secondLastHyphen);
Review Comment:
why not have a regex ? and extract using regex ?
i also think we should add get planId from the planTaskKey in a seperate
util method
##########
core/src/test/java/org/apache/iceberg/rest/TestInMemoryPlanningState.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.List;
+import org.apache.iceberg.FileScanTask;
+import org.apache.iceberg.exceptions.NoSuchPlanIdException;
+import org.apache.iceberg.exceptions.NoSuchPlanTaskException;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+public class TestInMemoryPlanningState {
Review Comment:
is it possible to add this in existing test for this ?
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -928,10 +928,19 @@ public static FetchScanTasksResponse fetchScanTasks(
Table table = catalog.loadTable(ident);
String planTask = request.planTask();
List<FileScanTask> fileScanTasks =
IN_MEMORY_PLANNING_STATE.fileScanTasksForPlanTask(planTask);
+ List<String> nextPlanTasks =
IN_MEMORY_PLANNING_STATE.nextPlanTask(planTask);
+
+ // Release the fetched plan task's state to prevent unbounded memory
growth. The FileScanTask
+ // list is the dominant memory consumer and need not be retained once
served (#17427). When
Review Comment:
does this needs to be in this comments ?
##########
core/src/main/java/org/apache/iceberg/rest/InMemoryPlanningState.java:
##########
@@ -127,6 +127,41 @@ List<String> nextPlanTask(String planTaskKey) {
return ImmutableList.of();
}
+ /**
+ * Releases the state for a single fetched plan task. Called after a
successful fetch to prevent
+ * unbounded memory growth — the {@code FileScanTask} list is the dominant
memory consumer and
+ * need not be retained once the client has received it (#17427).
+ *
+ * @param planTaskKey the plan task key to release
+ */
+ void releasePlanTask(String planTaskKey) {
+ planTaskToFileScanTasks.remove(planTaskKey);
+ planTaskToNext.remove(planTaskKey);
+ }
+
+ /**
+ * Releases the async planning state for the plan that owns the given plan
task key. Called when
+ * the last plan task in a chain is fetched, so completed plans don't
accumulate indefinitely
+ * (#17427).
Review Comment:
do we need to add this in the java doc ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
