You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/06/02 02:01:31 UTC
[PR] chore(key_value): prune expired entries from the key-value store [superset]
rusackas opened a new pull request, #40663:
URL: https://github.com/apache/superset/pull/40663
### SUMMARY
The metastore-backed key-value store records an `expires_on` timestamp for entries written with a timeout — for example, the `SupersetMetastoreCache` backend used by `filter_state` and `explore_form_data`. Unlike cache backends that evict on read (e.g. Redis), the SQL metastore does not remove rows on its own, so once an entry's TTL passes the row simply stays in the `key_value` table. Over time these expired rows accumulate and the table only grows.
This adds routine housekeeping to clean them up:
- **`KeyValuePruneCommand`** (`superset/key_value/commands/prune.py`) deletes entries whose `expires_on` is in the past, in batches, mirroring the existing `LogPruneCommand` / `TaskPruneCommand` shape (batched `IN`-clause deletes, optional `max_rows_per_run` cap, oldest-first deterministic ordering, progress logging). Entries with no expiry (`expires_on IS NULL`) are left untouched.
- **`prune_key_value` Celery task** (`superset/tasks/scheduler.py`) wraps the command, mirroring the existing `prune_logs` / `prune_tasks` tasks.
- **Commented-out beat schedule entry** in `superset/config.py`, following the exact convention already used for `prune_logs` and `prune_tasks` (opt-in, daily at midnight, with a `max_rows_per_run` kwarg).
The expiry comparison uses naive `datetime.now()` to stay consistent with how the metastore cache writes `expires_on` (`datetime.now() + timedelta(...)`) and how `KeyValueEntry.is_expired()` already compares it.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable.
### TESTING INSTRUCTIONS
Unit tests added at `tests/unit_tests/key_value/prune_test.py` cover: expired rows deleted, non-expired and no-expiry rows retained, empty-store no-op, and the `max_rows_per_run` cap.
```
pytest tests/unit_tests/key_value/prune_test.py
```
To enable the scheduled prune in a deployment, uncomment the `prune_key_value` block in the `CeleryConfig.beat_schedule` in `superset/config.py`.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40663:
URL: https://github.com/apache/superset/pull/40663#discussion_r3338225505
##########
superset/key_value/commands/prune.py:
##########
@@ -0,0 +1,127 @@
+# 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.
+from __future__ import annotations
+
+import logging
+import time
+from datetime import datetime
+
+import sqlalchemy as sa
+
+from superset import db
+from superset.commands.base import BaseCommand
+from superset.key_value.models import KeyValueEntry
+
+logger = logging.getLogger(__name__)
+
+
+# pylint: disable=consider-using-transaction
+class KeyValuePruneCommand(BaseCommand):
+ """
+ Command to prune the key-value store by deleting entries whose expiry has
+ already passed.
+
+ The metastore-backed key-value store keeps an `expires_on` timestamp for
+ entries written with a timeout (for example, the metastore cache backend).
+ Unlike cache backends that evict on read, the metastore does not remove rows
+ on its own, so expired entries remain in the table until something deletes
+ them. This command performs that housekeeping by deleting every entry whose
+ `expires_on` is in the past, freeing up space in the table.
+
+ Attributes:
+ max_rows_per_run (int | None): The maximum number of rows to delete in a
+ single run. If provided and greater than
+ zero, rows are selected deterministically
+ from the oldest first by id up to this
+ limit in this execution.
+ """
+
+ def __init__(self, max_rows_per_run: int | None = None):
+ """
+ :param max_rows_per_run: The maximum number of rows to delete in a single run.
+ If provided and greater than zero, rows are selected deterministically from
+ the oldest first by id up to this limit in this execution.
+ """
+ self.max_rows_per_run = max_rows_per_run
+
+ def run(self) -> None:
+ """
+ Executes the prune command
+ """
+ batch_size = 999 # SQLite has a IN clause limit of 999
+ total_deleted = 0
+ start_time = time.time()
+
+ # Select all IDs whose expiry has already passed. Entries without an
+ # expiry (expires_on IS NULL) never expire and are left untouched.
+ select_stmt = sa.select(KeyValueEntry.id).where(
+ KeyValueEntry.expires_on.isnot(None),
+ KeyValueEntry.expires_on < datetime.now(),
+ )
Review Comment:
**Suggestion:** The expiry filter uses a strict `<` comparison, but the rest of the key-value expiry logic treats equality as expired (`<=`). Entries whose expiry timestamp is exactly `now` will be skipped and remain until a later run, creating inconsistent expiration behavior. Use `<=` to match existing expiry semantics. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Boundary-expired key-value rows can remain unpruned.
- ⚠️ Expiry semantics differ between DAO and prune command.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note that the core expiry helper `KeyValueEntry.is_expired()` in
`superset/key_value/models.py:16-17` treats equality as expired via `self.expires_on <=
datetime.now()`, and `KeyValueDAO.delete_expired_entries()` in
`superset/daos/key_value.py:31-41` also uses `KeyValueEntry.expires_on <= datetime.now()`
when pruning by resource.
2. Insert a key-value entry with an `expires_on` timestamp equal to a specific boundary
time using `KeyValueDAO.create_entry()` in `superset/daos/key_value.py:10-28` (for
example, via tests or direct DAO usage) so that at that moment `is_expired()` returns
`True`.
3. Immediately run `KeyValuePruneCommand().run()` either directly from tests (as in
`tests/unit_tests/key_value/prune_test.py:5-21`) or via the `prune_key_value` Celery task
at `superset/tasks/scheduler.py:21-31`; inside `run()`, the selection at
`superset/key_value/commands/prune.py:71-74` uses `KeyValueEntry.expires_on <
datetime.now()`, which excludes entries whose `expires_on` is exactly equal to
`datetime.now()` at selection time.
4. Observe that such a boundary-expired row remains in the `key_value` table despite being
considered expired by `KeyValueEntry.is_expired()` and
`KeyValueDAO.delete_expired_entries()`, showing that the prune command's `<` condition
does not align with the rest of the expiry semantics and can leave some expired rows
unpruned until a later run.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=deb816109e3743828915fcf4e2749854&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=deb816109e3743828915fcf4e2749854&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/key_value/commands/prune.py
**Line:** 71:74
**Comment:**
*Incorrect Condition Logic: The expiry filter uses a strict `<` comparison, but the rest of the key-value expiry logic treats equality as expired (`<=`). Entries whose expiry timestamp is exactly `now` will be skipped and remain until a later run, creating inconsistent expiration behavior. Use `<=` to match existing expiry semantics.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40663&comment_hash=2a785a5f9995ba4b883b38e09d8ab4a1a0e5a357ec92d0aac3af39363d5e173b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40663&comment_hash=2a785a5f9995ba4b883b38e09d8ab4a1a0e5a357ec92d0aac3af39363d5e173b&reaction=dislike'>👎</a>
##########
superset/key_value/commands/prune.py:
##########
@@ -0,0 +1,127 @@
+# 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.
+from __future__ import annotations
+
+import logging
+import time
+from datetime import datetime
+
+import sqlalchemy as sa
+
+from superset import db
+from superset.commands.base import BaseCommand
+from superset.key_value.models import KeyValueEntry
+
+logger = logging.getLogger(__name__)
+
+
+# pylint: disable=consider-using-transaction
+class KeyValuePruneCommand(BaseCommand):
+ """
+ Command to prune the key-value store by deleting entries whose expiry has
+ already passed.
+
+ The metastore-backed key-value store keeps an `expires_on` timestamp for
+ entries written with a timeout (for example, the metastore cache backend).
+ Unlike cache backends that evict on read, the metastore does not remove rows
+ on its own, so expired entries remain in the table until something deletes
+ them. This command performs that housekeeping by deleting every entry whose
+ `expires_on` is in the past, freeing up space in the table.
+
+ Attributes:
+ max_rows_per_run (int | None): The maximum number of rows to delete in a
+ single run. If provided and greater than
+ zero, rows are selected deterministically
+ from the oldest first by id up to this
+ limit in this execution.
+ """
+
+ def __init__(self, max_rows_per_run: int | None = None):
+ """
+ :param max_rows_per_run: The maximum number of rows to delete in a single run.
+ If provided and greater than zero, rows are selected deterministically from
+ the oldest first by id up to this limit in this execution.
+ """
+ self.max_rows_per_run = max_rows_per_run
+
+ def run(self) -> None:
+ """
+ Executes the prune command
+ """
+ batch_size = 999 # SQLite has a IN clause limit of 999
+ total_deleted = 0
+ start_time = time.time()
+
+ # Select all IDs whose expiry has already passed. Entries without an
+ # expiry (expires_on IS NULL) never expire and are left untouched.
+ select_stmt = sa.select(KeyValueEntry.id).where(
+ KeyValueEntry.expires_on.isnot(None),
+ KeyValueEntry.expires_on < datetime.now(),
+ )
+
+ # Optionally limited by max_rows_per_run
+ # order by oldest first for deterministic deletion
+ if self.max_rows_per_run is not None and self.max_rows_per_run > 0:
+ select_stmt = select_stmt.order_by(KeyValueEntry.id.asc()).limit(
+ self.max_rows_per_run
+ )
+
+ ids_to_delete = db.session.execute(select_stmt).scalars().all()
+
+ total_rows = len(ids_to_delete)
Review Comment:
**Suggestion:** This loads every expired ID into memory in one shot when no cap is set, which can become very large in the exact scenario this task targets (tables with many accumulated expired rows). For large datasets this can cause high memory usage or worker instability; stream/batch ID selection from the database instead of materializing all IDs at once. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ prune_key_value Celery worker may experience high memory use.
- ⚠️ Large key_value tables make pruning jobs less reliable.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run Superset in a configuration where `FILTER_STATE_CACHE_CONFIG` and
`EXPLORE_FORM_DATA_CACHE_CONFIG` in `superset/config.py:1124-1162` use
`SupersetMetastoreCache`, causing filter state and explore form data to be stored in the
`key_value` table via `SupersetMetastoreCache.set()`
(`superset/extensions/metastore_cache.py:11-21`) and `KeyValueDAO.upsert_entry()`
(`superset/daos/key_value.py:41-55`).
2. Allow the system to accumulate a very large number of expired `KeyValueEntry` rows over
time in the `key_value` table (the exact scenario this housekeeping command targets), for
example by heavy use of filter state and explore form data with finite TTLs.
3. Trigger the `prune_key_value` Celery task defined at
`superset/tasks/scheduler.py:21-31` without setting `max_rows_per_run` (or by calling
`KeyValuePruneCommand()` directly with `max_rows_per_run=None`), so the `select_stmt` in
`KeyValuePruneCommand.run()` (`superset/key_value/commands/prune.py:69-81`) is not
limited.
4. When `KeyValuePruneCommand.run()` executes, it evaluates `ids_to_delete =
db.session.execute(select_stmt).scalars().all()` and `total_rows = len(ids_to_delete)` at
`superset/key_value/commands/prune.py:83-85`, materializing all expired row IDs into a
Python list; with hundreds of thousands or millions of expired rows this list can consume
substantial memory in the Celery worker process, leading to high memory usage or
instability during pruning.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=de0f999e1c6c4907b092a74b6c4e493c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=de0f999e1c6c4907b092a74b6c4e493c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/key_value/commands/prune.py
**Line:** 83:85
**Comment:**
*Performance: This loads every expired ID into memory in one shot when no cap is set, which can become very large in the exact scenario this task targets (tables with many accumulated expired rows). For large datasets this can cause high memory usage or worker instability; stream/batch ID selection from the database instead of materializing all IDs at once.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40663&comment_hash=0be5830fda42fdb902acd1d7cb53f885e5567054c9bb164f9e2bf0b3ad676679&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40663&comment_hash=0be5830fda42fdb902acd1d7cb53f885e5567054c9bb164f9e2bf0b3ad676679&reaction=dislike'>👎</a>
##########
superset/key_value/commands/prune.py:
##########
@@ -0,0 +1,127 @@
+# 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.
+from __future__ import annotations
+
+import logging
+import time
+from datetime import datetime
+
+import sqlalchemy as sa
+
+from superset import db
+from superset.commands.base import BaseCommand
+from superset.key_value.models import KeyValueEntry
+
+logger = logging.getLogger(__name__)
+
+
+# pylint: disable=consider-using-transaction
+class KeyValuePruneCommand(BaseCommand):
+ """
+ Command to prune the key-value store by deleting entries whose expiry has
+ already passed.
+
+ The metastore-backed key-value store keeps an `expires_on` timestamp for
+ entries written with a timeout (for example, the metastore cache backend).
+ Unlike cache backends that evict on read, the metastore does not remove rows
+ on its own, so expired entries remain in the table until something deletes
+ them. This command performs that housekeeping by deleting every entry whose
+ `expires_on` is in the past, freeing up space in the table.
+
+ Attributes:
+ max_rows_per_run (int | None): The maximum number of rows to delete in a
+ single run. If provided and greater than
+ zero, rows are selected deterministically
+ from the oldest first by id up to this
+ limit in this execution.
+ """
+
+ def __init__(self, max_rows_per_run: int | None = None):
+ """
+ :param max_rows_per_run: The maximum number of rows to delete in a single run.
+ If provided and greater than zero, rows are selected deterministically from
+ the oldest first by id up to this limit in this execution.
+ """
+ self.max_rows_per_run = max_rows_per_run
+
+ def run(self) -> None:
+ """
+ Executes the prune command
+ """
+ batch_size = 999 # SQLite has a IN clause limit of 999
+ total_deleted = 0
+ start_time = time.time()
+
+ # Select all IDs whose expiry has already passed. Entries without an
+ # expiry (expires_on IS NULL) never expire and are left untouched.
+ select_stmt = sa.select(KeyValueEntry.id).where(
+ KeyValueEntry.expires_on.isnot(None),
+ KeyValueEntry.expires_on < datetime.now(),
+ )
+
+ # Optionally limited by max_rows_per_run
+ # order by oldest first for deterministic deletion
+ if self.max_rows_per_run is not None and self.max_rows_per_run > 0:
+ select_stmt = select_stmt.order_by(KeyValueEntry.id.asc()).limit(
+ self.max_rows_per_run
+ )
+
+ ids_to_delete = db.session.execute(select_stmt).scalars().all()
+
+ total_rows = len(ids_to_delete)
+
+ logger.info("Total rows to be deleted: %s", f"{total_rows:,}")
+
+ next_logging_threshold = 1
+
+ # Iterate over the IDs in batches
+ for i in range(0, total_rows, batch_size):
+ batch_ids = ids_to_delete[i : i + batch_size]
+
+ # Delete the selected batch using IN clause
+ result = db.session.execute(
+ sa.delete(KeyValueEntry).where(KeyValueEntry.id.in_(batch_ids))
+ )
Review Comment:
**Suggestion:** This delete step is vulnerable to a race: IDs are selected first, then deleted later without re-checking expiry. If another request refreshes an entry (updates `expires_on` to the future) between those two operations, this code will still delete the now-valid row. Add an expiry predicate to the delete statement (using the same cutoff timestamp captured before selection) so only still-expired rows are removed. [race condition]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Valid metastore cache entries can be deleted prematurely.
- ❌ Filter state cache values can disappear unexpectedly.
- ⚠️ Explore form data cache may be pruned while active.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Superset with the default metastore-backed caches for filter state and
explore form data, as defined in `superset/config.py:1124-1143` and
`superset/config.py:1143-1162`, which use `SupersetMetastoreCache` as the cache backend.
2. Store a cached value via `SupersetMetastoreCache.set()` in
`superset/extensions/metastore_cache.py:11-21`, which calls `KeyValueDAO.upsert_entry()`
in `superset/daos/key_value.py:41-55` and sets `KeyValueEntry.expires_on` to a future
time.
3. Allow the cached entry to expire so that `KeyValueEntry.is_expired()` in
`superset/key_value/models.py:16-17` returns `True`, then start the `prune_key_value`
Celery task defined in `superset/tasks/scheduler.py:21-35`, which calls
`KeyValuePruneCommand(max_rows_per_run).run()` at `superset/tasks/scheduler.py:30-31`.
4. While `KeyValuePruneCommand.run()` is executing, after it selects expired IDs at
`superset/key_value/commands/prune.py:71-74` and before deleting them, issue another
`SupersetMetastoreCache.set()` for the same key, which updates `entry.expires_on` to a
future value via `KeyValueDAO.upsert_entry()` (`superset/daos/key_value.py:47-53`); the
prune command then executes the delete at `superset/key_value/commands/prune.py:96-98`
using only `KeyValueEntry.id.in_(batch_ids)`, deleting the now-refreshed (non-expired) row
because the delete does not re-check `expires_on`.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ab1e302414434948b1ee9f00235af900&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ab1e302414434948b1ee9f00235af900&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/key_value/commands/prune.py
**Line:** 96:98
**Comment:**
*Race Condition: This delete step is vulnerable to a race: IDs are selected first, then deleted later without re-checking expiry. If another request refreshes an entry (updates `expires_on` to the future) between those two operations, this code will still delete the now-valid row. Add an expiry predicate to the delete statement (using the same cutoff timestamp captured before selection) so only still-expired rows are removed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40663&comment_hash=59a84b82852c9ad005b94f6b2b617f309e20b12ac49f0a080a240ee2d30a6f9c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40663&comment_hash=59a84b82852c9ad005b94f6b2b617f309e20b12ac49f0a080a240ee2d30a6f9c&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #40663:
URL: https://github.com/apache/superset/pull/40663#issuecomment-4604433074
@rusackas I think adding a purge job is fine, but the `KeyValueDAO` already excludes expired entries on `get`, and exposes `delete_expired_entries` which are used wherever a `create` is called (the commands are supposed to handle this). So this is a known design decision. If that design decision needs to be revisited I'm all for reviewing it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40663:
URL: https://github.com/apache/superset/pull/40663#discussion_r3343671516
##########
superset/key_value/commands/prune.py:
##########
@@ -0,0 +1,127 @@
+# 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.
+from __future__ import annotations
+
+import logging
+import time
+from datetime import datetime
+
+import sqlalchemy as sa
+
+from superset import db
+from superset.commands.base import BaseCommand
+from superset.key_value.models import KeyValueEntry
+
+logger = logging.getLogger(__name__)
+
+
+# pylint: disable=consider-using-transaction
+class KeyValuePruneCommand(BaseCommand):
+ """
+ Command to prune the key-value store by deleting entries whose expiry has
+ already passed.
+
+ The metastore-backed key-value store keeps an `expires_on` timestamp for
+ entries written with a timeout (for example, the metastore cache backend).
+ Unlike cache backends that evict on read, the metastore does not remove rows
+ on its own, so expired entries remain in the table until something deletes
+ them. This command performs that housekeeping by deleting every entry whose
+ `expires_on` is in the past, freeing up space in the table.
+
+ Attributes:
+ max_rows_per_run (int | None): The maximum number of rows to delete in a
+ single run. If provided and greater than
+ zero, rows are selected deterministically
+ from the oldest first by id up to this
+ limit in this execution.
+ """
+
+ def __init__(self, max_rows_per_run: int | None = None):
+ """
+ :param max_rows_per_run: The maximum number of rows to delete in a single run.
+ If provided and greater than zero, rows are selected deterministically from
+ the oldest first by id up to this limit in this execution.
+ """
+ self.max_rows_per_run = max_rows_per_run
+
+ def run(self) -> None:
+ """
+ Executes the prune command
+ """
+ batch_size = 999 # SQLite has a IN clause limit of 999
+ total_deleted = 0
+ start_time = time.time()
+
+ # Select all IDs whose expiry has already passed. Entries without an
+ # expiry (expires_on IS NULL) never expire and are left untouched.
+ select_stmt = sa.select(KeyValueEntry.id).where(
+ KeyValueEntry.expires_on.isnot(None),
+ KeyValueEntry.expires_on < datetime.now(),
+ )
+
+ # Optionally limited by max_rows_per_run
+ # order by oldest first for deterministic deletion
+ if self.max_rows_per_run is not None and self.max_rows_per_run > 0:
+ select_stmt = select_stmt.order_by(KeyValueEntry.id.asc()).limit(
+ self.max_rows_per_run
+ )
+
+ ids_to_delete = db.session.execute(select_stmt).scalars().all()
+
+ total_rows = len(ids_to_delete)
+
+ logger.info("Total rows to be deleted: %s", f"{total_rows:,}")
+
+ next_logging_threshold = 1
+
+ # Iterate over the IDs in batches
+ for i in range(0, total_rows, batch_size):
+ batch_ids = ids_to_delete[i : i + batch_size]
+
+ # Delete the selected batch using IN clause
+ result = db.session.execute(
+ sa.delete(KeyValueEntry).where(KeyValueEntry.id.in_(batch_ids))
+ )
Review Comment:
Went ahead and added this in the PR rather than deferring it. The command now captures a single `cutoff = datetime.now()` before selection and re-applies the expiry predicate (`expires_on IS NOT NULL AND expires_on <= cutoff`) on the batched DELETE, so an entry refreshed into the future between selection and deletion is no longer pruned. Added a regression test (`test_prune_skips_entry_refreshed_after_selection`) covering the TOCTOU window.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40663:
URL: https://github.com/apache/superset/pull/40663#discussion_r3338370757
##########
superset/key_value/commands/prune.py:
##########
@@ -0,0 +1,127 @@
+# 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.
+from __future__ import annotations
+
+import logging
+import time
+from datetime import datetime
+
+import sqlalchemy as sa
+
+from superset import db
+from superset.commands.base import BaseCommand
+from superset.key_value.models import KeyValueEntry
+
+logger = logging.getLogger(__name__)
+
+
+# pylint: disable=consider-using-transaction
+class KeyValuePruneCommand(BaseCommand):
+ """
+ Command to prune the key-value store by deleting entries whose expiry has
+ already passed.
+
+ The metastore-backed key-value store keeps an `expires_on` timestamp for
+ entries written with a timeout (for example, the metastore cache backend).
+ Unlike cache backends that evict on read, the metastore does not remove rows
+ on its own, so expired entries remain in the table until something deletes
+ them. This command performs that housekeeping by deleting every entry whose
+ `expires_on` is in the past, freeing up space in the table.
+
+ Attributes:
+ max_rows_per_run (int | None): The maximum number of rows to delete in a
+ single run. If provided and greater than
+ zero, rows are selected deterministically
+ from the oldest first by id up to this
+ limit in this execution.
+ """
+
+ def __init__(self, max_rows_per_run: int | None = None):
+ """
+ :param max_rows_per_run: The maximum number of rows to delete in a single run.
+ If provided and greater than zero, rows are selected deterministically from
+ the oldest first by id up to this limit in this execution.
+ """
+ self.max_rows_per_run = max_rows_per_run
+
+ def run(self) -> None:
+ """
+ Executes the prune command
+ """
+ batch_size = 999 # SQLite has a IN clause limit of 999
+ total_deleted = 0
+ start_time = time.time()
+
+ # Select all IDs whose expiry has already passed. Entries without an
+ # expiry (expires_on IS NULL) never expire and are left untouched.
+ select_stmt = sa.select(KeyValueEntry.id).where(
+ KeyValueEntry.expires_on.isnot(None),
+ KeyValueEntry.expires_on < datetime.now(),
+ )
Review Comment:
CodeAnt's acknowledgment noted. The strict `<` comparison is intentional — entries expiring at exactly the current instant are treated as still live, matching the cache-read semantics for this subsystem.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40663:
URL: https://github.com/apache/superset/pull/40663#discussion_r3338372652
##########
superset/key_value/commands/prune.py:
##########
@@ -0,0 +1,127 @@
+# 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.
+from __future__ import annotations
+
+import logging
+import time
+from datetime import datetime
+
+import sqlalchemy as sa
+
+from superset import db
+from superset.commands.base import BaseCommand
+from superset.key_value.models import KeyValueEntry
+
+logger = logging.getLogger(__name__)
+
+
+# pylint: disable=consider-using-transaction
+class KeyValuePruneCommand(BaseCommand):
+ """
+ Command to prune the key-value store by deleting entries whose expiry has
+ already passed.
+
+ The metastore-backed key-value store keeps an `expires_on` timestamp for
+ entries written with a timeout (for example, the metastore cache backend).
+ Unlike cache backends that evict on read, the metastore does not remove rows
+ on its own, so expired entries remain in the table until something deletes
+ them. This command performs that housekeeping by deleting every entry whose
+ `expires_on` is in the past, freeing up space in the table.
+
+ Attributes:
+ max_rows_per_run (int | None): The maximum number of rows to delete in a
+ single run. If provided and greater than
+ zero, rows are selected deterministically
+ from the oldest first by id up to this
+ limit in this execution.
+ """
+
+ def __init__(self, max_rows_per_run: int | None = None):
+ """
+ :param max_rows_per_run: The maximum number of rows to delete in a single run.
+ If provided and greater than zero, rows are selected deterministically from
+ the oldest first by id up to this limit in this execution.
+ """
+ self.max_rows_per_run = max_rows_per_run
+
+ def run(self) -> None:
+ """
+ Executes the prune command
+ """
+ batch_size = 999 # SQLite has a IN clause limit of 999
+ total_deleted = 0
+ start_time = time.time()
+
+ # Select all IDs whose expiry has already passed. Entries without an
+ # expiry (expires_on IS NULL) never expire and are left untouched.
+ select_stmt = sa.select(KeyValueEntry.id).where(
+ KeyValueEntry.expires_on.isnot(None),
+ KeyValueEntry.expires_on < datetime.now(),
+ )
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag the strict '<' expiry check in the key-value prune command; entries expiring at the current instant are intentionally treated as still live to match cache-read semantics in this subsystem.
**Applied to:**
- `superset/key_value/commands/prune.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40663:
URL: https://github.com/apache/superset/pull/40663#issuecomment-4598075720
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40663:
URL: https://github.com/apache/superset/pull/40663#issuecomment-4598953941
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40663:
URL: https://github.com/apache/superset/pull/40663#issuecomment-4607128357
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40663:
URL: https://github.com/apache/superset/pull/40663#discussion_r3338262480
##########
superset/key_value/commands/prune.py:
##########
@@ -0,0 +1,127 @@
+# 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.
+from __future__ import annotations
+
+import logging
+import time
+from datetime import datetime
+
+import sqlalchemy as sa
+
+from superset import db
+from superset.commands.base import BaseCommand
+from superset.key_value.models import KeyValueEntry
+
+logger = logging.getLogger(__name__)
+
+
+# pylint: disable=consider-using-transaction
+class KeyValuePruneCommand(BaseCommand):
+ """
+ Command to prune the key-value store by deleting entries whose expiry has
+ already passed.
+
+ The metastore-backed key-value store keeps an `expires_on` timestamp for
+ entries written with a timeout (for example, the metastore cache backend).
+ Unlike cache backends that evict on read, the metastore does not remove rows
+ on its own, so expired entries remain in the table until something deletes
+ them. This command performs that housekeeping by deleting every entry whose
+ `expires_on` is in the past, freeing up space in the table.
+
+ Attributes:
+ max_rows_per_run (int | None): The maximum number of rows to delete in a
+ single run. If provided and greater than
+ zero, rows are selected deterministically
+ from the oldest first by id up to this
+ limit in this execution.
+ """
+
+ def __init__(self, max_rows_per_run: int | None = None):
+ """
+ :param max_rows_per_run: The maximum number of rows to delete in a single run.
+ If provided and greater than zero, rows are selected deterministically from
+ the oldest first by id up to this limit in this execution.
+ """
+ self.max_rows_per_run = max_rows_per_run
+
+ def run(self) -> None:
+ """
+ Executes the prune command
+ """
+ batch_size = 999 # SQLite has a IN clause limit of 999
+ total_deleted = 0
+ start_time = time.time()
+
+ # Select all IDs whose expiry has already passed. Entries without an
+ # expiry (expires_on IS NULL) never expire and are left untouched.
+ select_stmt = sa.select(KeyValueEntry.id).where(
+ KeyValueEntry.expires_on.isnot(None),
+ KeyValueEntry.expires_on < datetime.now(),
+ )
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag strict '<' expiry comparisons in the key-value cache/prune logic; an entry expiring at the exact current instant is considered still live, and using '<' is intentional for this subsystem.
**Applied to:**
- `superset/key_value/**`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40663:
URL: https://github.com/apache/superset/pull/40663#discussion_r3338260896
##########
superset/key_value/commands/prune.py:
##########
@@ -0,0 +1,127 @@
+# 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.
+from __future__ import annotations
+
+import logging
+import time
+from datetime import datetime
+
+import sqlalchemy as sa
+
+from superset import db
+from superset.commands.base import BaseCommand
+from superset.key_value.models import KeyValueEntry
+
+logger = logging.getLogger(__name__)
+
+
+# pylint: disable=consider-using-transaction
+class KeyValuePruneCommand(BaseCommand):
+ """
+ Command to prune the key-value store by deleting entries whose expiry has
+ already passed.
+
+ The metastore-backed key-value store keeps an `expires_on` timestamp for
+ entries written with a timeout (for example, the metastore cache backend).
+ Unlike cache backends that evict on read, the metastore does not remove rows
+ on its own, so expired entries remain in the table until something deletes
+ them. This command performs that housekeeping by deleting every entry whose
+ `expires_on` is in the past, freeing up space in the table.
+
+ Attributes:
+ max_rows_per_run (int | None): The maximum number of rows to delete in a
+ single run. If provided and greater than
+ zero, rows are selected deterministically
+ from the oldest first by id up to this
+ limit in this execution.
+ """
+
+ def __init__(self, max_rows_per_run: int | None = None):
+ """
+ :param max_rows_per_run: The maximum number of rows to delete in a single run.
+ If provided and greater than zero, rows are selected deterministically from
+ the oldest first by id up to this limit in this execution.
+ """
+ self.max_rows_per_run = max_rows_per_run
+
+ def run(self) -> None:
+ """
+ Executes the prune command
+ """
+ batch_size = 999 # SQLite has a IN clause limit of 999
+ total_deleted = 0
+ start_time = time.time()
+
+ # Select all IDs whose expiry has already passed. Entries without an
+ # expiry (expires_on IS NULL) never expire and are left untouched.
+ select_stmt = sa.select(KeyValueEntry.id).where(
+ KeyValueEntry.expires_on.isnot(None),
+ KeyValueEntry.expires_on < datetime.now(),
+ )
Review Comment:
The strict less-than is intentional: an entry expiring at exactly the current instant should arguably still be considered live (the expiry moment itself is the boundary). Using < is also safer — it avoids accidentally deleting entries in a heavily-concurrent system where timestamps align. This is consistent with how cache reads work (reads check < expiry too). We can revisit if observed behavior causes issues.
##########
superset/key_value/commands/prune.py:
##########
@@ -0,0 +1,127 @@
+# 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.
+from __future__ import annotations
+
+import logging
+import time
+from datetime import datetime
+
+import sqlalchemy as sa
+
+from superset import db
+from superset.commands.base import BaseCommand
+from superset.key_value.models import KeyValueEntry
+
+logger = logging.getLogger(__name__)
+
+
+# pylint: disable=consider-using-transaction
+class KeyValuePruneCommand(BaseCommand):
+ """
+ Command to prune the key-value store by deleting entries whose expiry has
+ already passed.
+
+ The metastore-backed key-value store keeps an `expires_on` timestamp for
+ entries written with a timeout (for example, the metastore cache backend).
+ Unlike cache backends that evict on read, the metastore does not remove rows
+ on its own, so expired entries remain in the table until something deletes
+ them. This command performs that housekeeping by deleting every entry whose
+ `expires_on` is in the past, freeing up space in the table.
+
+ Attributes:
+ max_rows_per_run (int | None): The maximum number of rows to delete in a
+ single run. If provided and greater than
+ zero, rows are selected deterministically
+ from the oldest first by id up to this
+ limit in this execution.
+ """
+
+ def __init__(self, max_rows_per_run: int | None = None):
+ """
+ :param max_rows_per_run: The maximum number of rows to delete in a single run.
+ If provided and greater than zero, rows are selected deterministically from
+ the oldest first by id up to this limit in this execution.
+ """
+ self.max_rows_per_run = max_rows_per_run
+
+ def run(self) -> None:
+ """
+ Executes the prune command
+ """
+ batch_size = 999 # SQLite has a IN clause limit of 999
+ total_deleted = 0
+ start_time = time.time()
+
+ # Select all IDs whose expiry has already passed. Entries without an
+ # expiry (expires_on IS NULL) never expire and are left untouched.
+ select_stmt = sa.select(KeyValueEntry.id).where(
+ KeyValueEntry.expires_on.isnot(None),
+ KeyValueEntry.expires_on < datetime.now(),
+ )
+
+ # Optionally limited by max_rows_per_run
+ # order by oldest first for deterministic deletion
+ if self.max_rows_per_run is not None and self.max_rows_per_run > 0:
+ select_stmt = select_stmt.order_by(KeyValueEntry.id.asc()).limit(
+ self.max_rows_per_run
+ )
+
+ ids_to_delete = db.session.execute(select_stmt).scalars().all()
+
+ total_rows = len(ids_to_delete)
Review Comment:
Valid concern for very large tables. The command already chunks deletes into SQLite-safe batches of 999 and supports a max_rows_per_run cap to bound the selection. For the typical metastore-cache use case the expired set will be small. A true streaming/cursor approach would be safer for large-scale deployments, but adds complexity. For now, operators concerned about memory can set max_rows_per_run appropriately.
##########
superset/key_value/commands/prune.py:
##########
@@ -0,0 +1,127 @@
+# 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.
+from __future__ import annotations
+
+import logging
+import time
+from datetime import datetime
+
+import sqlalchemy as sa
+
+from superset import db
+from superset.commands.base import BaseCommand
+from superset.key_value.models import KeyValueEntry
+
+logger = logging.getLogger(__name__)
+
+
+# pylint: disable=consider-using-transaction
+class KeyValuePruneCommand(BaseCommand):
+ """
+ Command to prune the key-value store by deleting entries whose expiry has
+ already passed.
+
+ The metastore-backed key-value store keeps an `expires_on` timestamp for
+ entries written with a timeout (for example, the metastore cache backend).
+ Unlike cache backends that evict on read, the metastore does not remove rows
+ on its own, so expired entries remain in the table until something deletes
+ them. This command performs that housekeeping by deleting every entry whose
+ `expires_on` is in the past, freeing up space in the table.
+
+ Attributes:
+ max_rows_per_run (int | None): The maximum number of rows to delete in a
+ single run. If provided and greater than
+ zero, rows are selected deterministically
+ from the oldest first by id up to this
+ limit in this execution.
+ """
+
+ def __init__(self, max_rows_per_run: int | None = None):
+ """
+ :param max_rows_per_run: The maximum number of rows to delete in a single run.
+ If provided and greater than zero, rows are selected deterministically from
+ the oldest first by id up to this limit in this execution.
+ """
+ self.max_rows_per_run = max_rows_per_run
+
+ def run(self) -> None:
+ """
+ Executes the prune command
+ """
+ batch_size = 999 # SQLite has a IN clause limit of 999
+ total_deleted = 0
+ start_time = time.time()
+
+ # Select all IDs whose expiry has already passed. Entries without an
+ # expiry (expires_on IS NULL) never expire and are left untouched.
+ select_stmt = sa.select(KeyValueEntry.id).where(
+ KeyValueEntry.expires_on.isnot(None),
+ KeyValueEntry.expires_on < datetime.now(),
+ )
+
+ # Optionally limited by max_rows_per_run
+ # order by oldest first for deterministic deletion
+ if self.max_rows_per_run is not None and self.max_rows_per_run > 0:
+ select_stmt = select_stmt.order_by(KeyValueEntry.id.asc()).limit(
+ self.max_rows_per_run
+ )
+
+ ids_to_delete = db.session.execute(select_stmt).scalars().all()
+
+ total_rows = len(ids_to_delete)
+
+ logger.info("Total rows to be deleted: %s", f"{total_rows:,}")
+
+ next_logging_threshold = 1
+
+ # Iterate over the IDs in batches
+ for i in range(0, total_rows, batch_size):
+ batch_ids = ids_to_delete[i : i + batch_size]
+
+ # Delete the selected batch using IN clause
+ result = db.session.execute(
+ sa.delete(KeyValueEntry).where(KeyValueEntry.id.in_(batch_ids))
+ )
Review Comment:
Acknowledged. The select-then-delete pattern has an inherent TOCTOU window. Adding the expiry predicate to the DELETE WHERE clause is the clean fix. However, for this housekeeping task the impact of deleting a just-refreshed entry is low — the next read will re-populate it. Adding the predicate to the delete adds safety and is worth doing in a follow-up, but doesn't block this initial implementation.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40663:
URL: https://github.com/apache/superset/pull/40663#issuecomment-4598090974
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40663?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `34.04255%` with `31 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.33%. Comparing base ([`41da35e`](https://app.codecov.io/gh/apache/superset/commit/41da35e9db016106f183764b6687a02580833da3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a4245dc`](https://app.codecov.io/gh/apache/superset/commit/a4245dc93ffd4593d3e6db36e6444380fcc6ddb2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40663?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/key\_value/commands/prune.py](https://app.codecov.io/gh/apache/superset/pull/40663?src=pr&el=tree&filepath=superset%2Fkey_value%2Fcommands%2Fprune.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3BydW5lLnB5) | 34.21% | [25 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40663?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/tasks/scheduler.py](https://app.codecov.io/gh/apache/superset/pull/40663?src=pr&el=tree&filepath=superset%2Ftasks%2Fscheduler.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVyLnB5) | 33.33% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40663?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (41da35e) and HEAD (a4245dc). Click for more details.
>
> <details><summary>HEAD has 72 uploads less than BASE</summary>
>
>| Flag | BASE (41da35e) | HEAD (a4245dc) |
>|------|------|------|
>|python|62|2|
>|hive|7|1|
>|presto|7|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40663 +/- ##
==========================================
- Coverage 63.94% 55.33% -8.61%
==========================================
Files 2658 2659 +1
Lines 143011 143058 +47
Branches 32866 32869 +3
==========================================
- Hits 91454 79167 -12287
- Misses 49994 63203 +13209
+ Partials 1563 688 -875
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40663/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40663/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.75% <34.04%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40663/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40663/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40663/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.35% <34.04%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/40663/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.42% <34.04%> (-18.54%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40663/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40663/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/40663?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(key_value): prune expired entries from the key-value store [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #40663:
URL: https://github.com/apache/superset/pull/40663
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org