Add storage command for triggering RocksDB compaction by david-wagih · Pull Request #4385 · restatedev/restate

david-wagih · 2026-02-10T19:21:32Z

What is Added

Introduced TriggerCompaction RPC in the NodeCtlSvc protocol.
Implemented trigger_compaction method in NodeCtlSvcHandler to handle compaction requests.
Added Compact command in the Restate CLI for initiating compaction on specified nodes.
Created compact.rs to define compaction options and logic.
Updated module structure to include storage commands.

github-actions · 2026-02-10T19:22:01Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

david-wagih · 2026-02-10T19:22:04Z

#4238

david-wagih · 2026-02-10T19:23:25Z

I have read the CLA Document and I hereby sign the CLA

david-wagih · 2026-02-10T19:24:06Z

recheck

david-wagih · 2026-02-21T14:34:01Z

@tillrohrmann
Hi Till, just asking for a feedback on this one

tillrohrmann · 2026-02-23T09:51:16Z

Hi @david-wagih, thanks a lot for this contribution. I'll get to review your PR this week. Sorry for the slow response.

david-wagih · 2026-03-11T22:10:24Z

@tillrohrmann Hi Till, I am just rechecking with you about the state of this PR

tillrohrmann · 2026-03-12T10:34:13Z

Hi @david-wagih, sorry for the delay. I haven't forgotten it. Will get to it swiftly.

tillrohrmann

Thanks a lot for creating this PR @david-wagih. I think it looks good. I left a few comments for how we could improve it. Let me know what you think.

tillrohrmann · 2026-03-12T14:29:45Z

+    } else if name == "db" || name.starts_with("db-") {
+        DatabaseKind::PartitionStore
+    } else {
+        DatabaseKind::Unknown


What is this variant being used for? It looks as if all unknown dbs map to this variant. Since we are translating the existing db names to variants it could then happen that a request for "foobar" triggers a compaction on a newly introduced db for which we forgot to add the translation to DatabaseKind.

Differently asked, why not ignoring requested db to compact which are not known?

tillrohrmann · 2026-03-12T14:32:38Z

+            }
+
+            let cf_count = db.cfs().len() as u32;
+            db.compact_all().await;


I assume it was intentional to run compactions sequentially to avoid overloading the system with I/O operations. Maybe add a comment to make this explicit (why we are only running a single compaction at a time).

tillrohrmann · 2026-03-12T14:34:22Z

+            }
+
+            let cf_count = db.cfs().len() as u32;
+            db.compact_all().await;


Maybe we can extend compact_all to return whether the compaction was successful. Right now, independent of whether the compaction succeeded, we are returning success: true.

tillrohrmann · 2026-03-12T14:38:27Z

+    if name == "log-server" {
+        DatabaseKind::LogServer
+    } else if name == "replicated-metadata-server" {
+        DatabaseKind::MetadataServer
+    } else if name == "local-loglet" {
+        DatabaseKind::LocalLoglet
+    } else if name == "db" || name.starts_with("db-") {


There is a chance that this will silently break if the db names should be changed where we open the RocksDB databases.

tillrohrmann · 2026-03-12T14:40:05Z

+        let requested_kinds: Vec<DatabaseKind> = if request.databases.is_empty() {
+            vec![DatabaseKind::All]


If an empty request.databases translates into all databases, do we need an explicit DatabaseKind::All variant? It seems that we are mixing the database kinds with more general filter conditions by having a DatabaseKind::All.

tillrohrmann · 2026-03-12T14:41:06Z

+    /// Database type(s) to compact: all, partition-store, log-server,
+    /// metadata-server, local-loglet. Defaults to all.
+    #[arg(long, short = 'd', value_delimiter = ',')]
+    database: Vec<String>,


Could the Vec type be DatabaseKind? That way we might get automatic type safety and prevent user typos from going unnoticed.

tillrohrmann · 2026-03-12T14:45:28Z

+    /// Target specific node addresses (default: all nodes from cluster)
+    #[arg(long, short = 'n', value_delimiter = ',')]
+    node: Vec<AdvertisedAddress<FabricPort>>,


Wondering whether we want to rather specify NodeId instead of addresses. We can obtain the addresses from the NodesConfiguration. A bit similar to restatectl nodes rm --nodes N1,N2.

tillrohrmann · 2026-03-12T14:53:11Z

+    let mut client = new_node_ctl_client(channel, &CliContext::get().network);
+
+    let request = TriggerCompactionRequest { databases };
+    let response = tokio::time::timeout(COMPACTION_TIMEOUT, client.trigger_compaction(request))


The compaction timeout is hardcoded which can be a problem for very long running compaction calls. Could we use CliContext::get().request_timeout() instead?

tillrohrmann · 2026-03-12T14:54:11Z

+    } else {
+        opts.database
+            .iter()
+            .filter_map(|s| parse_database_kind(s))


Here we are silently dropping dbs with typos.

david-wagih · 2026-03-20T10:49:08Z

@tillrohrmann
thanks Till, for the review. I am checking the comments now and will push a new commit when done

david-wagih · 2026-03-23T09:27:50Z

@tillrohrmann
Hello Till, I have implemented the fixes for your comments. Can you please recheck this PR

tillrohrmann · 2026-03-23T09:30:25Z

Will do later today.

david-wagih · 2026-03-26T15:53:59Z

@tillrohrmann
Hello Till, I am just rechecking with you

tillrohrmann

Great work @david-wagih. The changes look good to me. I will try your changes out and if everything works, then I will merge your PR. Will also address the only comment I had left. Thanks a lot for your contribution ❤️

david-wagih · 2026-03-26T20:42:18Z

Great work @david-wagih. The changes look good to me. I will try your changes out and if everything works, then I will merge your PR. Will also address the only comment I had left. Thanks a lot for your contribution ❤️

thanks till for your feedback, excited to get my first contribution in restate merged ❤️

Address review feedback on storage compact command - Remove DatabaseKind::All from proto; empty databases list now means compact all, keeping DB kind semantics clean - Rename DATABASE_KIND_UNKNOWN to DATABASE_KIND_UNSPECIFIED (proto3 convention) - db_name_to_kind returns Option<DatabaseKind> so unrecognized DB names are skipped rather than risking accidental compaction of future DBs - compact_all now returns Result<(), RocksError> so failures are propagated into the per-DB success/error fields in the response instead of always reporting success: true - Add comment explaining sequential compaction is intentional to avoid I/O overload - CLI --database field is now Vec<DatabaseKind> with a value_parser so clap rejects invalid values at parse time instead of silently dropping them - CLI --node now accepts PlainNodeId (e.g. N1,N2) instead of raw addresses; addresses are resolved from NodesConfiguration, matching the nodes rm pattern - Replace hardcoded 300s compaction timeout with CliContext::get().request_timeout() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Update server/src/signal.rs Co-authored-by: Till Rohrmann <till.rohrmann@gmail.com>

Move DatabaseKind from node_ctl_svc.proto to common.proto so it is available in restate-types. Add DatabaseKind::db_name() as the canonical mapping from kind to RocksDB database name, and store the kind on DbSpec so that RocksDb::kind() can be queried directly without reverse- engineering the kind from the name string. This eliminates the scattered DB_NAME constants from individual crates and removes the fragile db_name_to_kind() function that could silently go out of sync if a database was renamed.

tillrohrmann self-requested a review February 10, 2026 19:53

tillrohrmann reviewed Mar 12, 2026

View reviewed changes

david-wagih requested a review from tillrohrmann March 20, 2026 11:18

tillrohrmann approved these changes Mar 26, 2026

View reviewed changes

Comment thread server/src/signal.rs Outdated

tillrohrmann force-pushed the feature/testatectl-command-compact-db branch from 2eb9c2d to 30d7bb1 Compare March 27, 2026 10:28

david-wagih and others added 2 commits March 27, 2026 11:56

tillrohrmann force-pushed the feature/testatectl-command-compact-db branch from 30d7bb1 to aa1749d Compare March 27, 2026 10:57

tillrohrmann merged commit aa1749d into restatedev:main Mar 27, 2026
1 check passed

github-actions Bot locked and limited conversation to collaborators Mar 27, 2026

		let requested_kinds: Vec<DatabaseKind> = if request.databases.is_empty() {
		vec![DatabaseKind::All]

Conversation

david-wagih commented Feb 10, 2026

What is Added

Uh oh!

github-actions Bot commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

david-wagih commented Feb 10, 2026

Uh oh!

david-wagih commented Feb 10, 2026

Uh oh!

david-wagih commented Feb 10, 2026

Uh oh!

david-wagih commented Feb 21, 2026

Uh oh!

tillrohrmann commented Feb 23, 2026

Uh oh!

david-wagih commented Mar 11, 2026

Uh oh!

tillrohrmann commented Mar 12, 2026

Uh oh!

tillrohrmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-wagih commented Mar 20, 2026

Uh oh!

david-wagih commented Mar 23, 2026

Uh oh!

tillrohrmann commented Mar 23, 2026

Uh oh!

david-wagih commented Mar 26, 2026

Uh oh!

tillrohrmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

david-wagih commented Mar 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Feb 10, 2026 •

edited

Loading