Skip to content

Conversation

@fm3
Copy link
Member

@fm3 fm3 commented Oct 28, 2025

This config option overwrote the value in postgres with every datastore ping. We want to be able to set it in the db permanently.

In fact, we set it to true in the test DB. This will be tested in the libs tests.

Steps to test

  • Set the flag to true in db. It should stay true even through wk restarts and 10min datastore ping interval.
  • Set the flag to false, should stay false.
  • CI/e2e-tests should be green

  • Added changelog entry (create a $PR_NUMBER.md file in unreleased_changes or use ./tools/create-changelog-entry.py)
  • Added migration guide entry if applicable (edit the same file as for the changelog)
  • Needs datastore update after deployment

@fm3 fm3 self-assigned this Oct 28, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

📝 Walkthrough

Walkthrough

This PR removes the reportUsedStorageEnabled config/field and related DAO update paths: the flag is now managed in PostgreSQL only. The DataStoreStatus API type no longer includes reportUsedStorageEnabled, and datastore config lines referencing the flag were removed or documented as database-controlled.

Changes

Cohort / File(s) Summary
Controller / DAO / API client
app/controllers/WKRemoteDataStoreController.scala, app/models/dataset/DataStore.scala, webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSRemoteWebknossosClient.scala
Removed DataStoreDAO.updateReportUsedStorageEnabledByName declaration and its invocation in the controller. Removed reportUsedStorageEnabled from the DataStoreStatus case class and updated payload generation/call sites. Minor logging string change in controller.
Configuration / Defaults
conf/application.conf, webknossos-datastore/conf/standalone-datastore.conf
Deleted reportUsedStorage.enabled config entry and added an inline comment clarifying that storage scanning is controlled via reportUsedStorageEnabled in PostgreSQL.
Docs / Release notes
unreleased_changes/9024.md
Documented removal of the config option and provided migration guidance to enable storage scan by setting reportUsedStorageEnabled = TRUE in webknossos.dataStores.
Datastore config additions
webknossos-datastore/app/com/scalableminds/webknossos/datastore/DataStoreConfig.scala
Added ImageArrayChunks entry under Cache children and added DataVaults to Datastore children; removed ReportUsedStorage config object.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Changes are cohesive but span multiple layers (controller, DAO, API client, config, release notes).
  • Mostly removals and small signature changes; verify all call sites and serialization boundaries.
  • Areas to pay extra attention:
    • Ensure no remaining references to updateReportUsedStorageEnabledByName or reportUsedStorageEnabled across services.
    • Verify remote client payloads and any JSON (de)serialization schemas affected by the DataStoreStatus signature change.
    • Confirm configuration parsing and defaults after removal of reportUsedStorage.enabled.

Possibly related PRs

Suggested reviewers

  • MichaelBuessemeyer
  • frcroth

Poem

🐰
A flag hopped off into PostgreSQL night,
No longer in files or controller sight.
Fields trimmed, configs tidy and neat,
Datastores whisper, "Now we're discreet." 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "Remove Datastore config reportUsedStorage.enabled. Set in DB instead" directly and accurately reflects the main objective of the changeset. The title clearly identifies the primary change—removing a configuration option and moving the responsibility to the database—which is supported by the modifications across multiple files including removal of the config lines, the DAO method, and the DataStoreStatus field. The title is concise, specific, and provides sufficient clarity for someone reviewing the repository history to understand the core intent of the change.
Description Check ✅ Passed The pull request description is clearly related to the changeset and provides meaningful context about the changes. It explains the rationale for the modification—that the configuration option was overwriting the database value on every datastore ping—and articulates the desired behavior of permanently setting the flag in the database. The description also outlines concrete testing steps and confirms that necessary documentation and migration guidance have been added, all of which align directly with the modifications present in the changeset.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch measure-storage-db-only

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 209ffce and 19bb438.

📒 Files selected for processing (1)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/DataStoreConfig.scala (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-smoketest-push
  • GitHub Check: backend-tests
  • GitHub Check: frontend-tests
🔇 Additional comments (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/DataStoreConfig.scala (3)

55-66: LGTM: ReportUsedStorage correctly removed.

The removal of the ReportUsedStorage configuration object aligns with the PR objective to manage this flag in the database instead of config. The Datastore.children list has been properly updated to exclude it.


36-38: ImageArrayChunks configuration is properly defined—no action needed.

The configuration datastore.cache.imageArrayChunks.maxSizeBytes exists in both webknossos-datastore/conf/standalone-datastore.conf (line 66) and conf/application.conf (line 226), both set to 2000000000 (2 GB). The code implementation correctly references this existing configuration and follows the established pattern for cache configuration objects.


58-60: The dataVaults configuration exists and is properly referenced in the code.

The configuration path datastore.dataVaults.credentials correctly maps to the existing dataVaults block in the config files (application.conf, standalone-datastore.conf, datastore-docker.conf), where it's defined as credentials = []. The implementation follows the same pattern as other datastore configuration objects and is correctly integrated into the codebase.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@fm3 fm3 changed the title Remove Datastore config option reportUsedStorageEnabled. Set in DB instead Remove Datastore config reportUsedStorage.enabled. Set in DB instead Oct 28, 2025
@fm3 fm3 marked this pull request as ready for review October 28, 2025 10:18
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 64e72fd and 209ffce.

⛔ Files ignored due to path filters (1)
  • test/db/dataStores.csv is excluded by !**/*.csv
📒 Files selected for processing (6)
  • app/controllers/WKRemoteDataStoreController.scala (1 hunks)
  • app/models/dataset/DataStore.scala (0 hunks)
  • conf/application.conf (1 hunks)
  • unreleased_changes/9024.md (1 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSRemoteWebknossosClient.scala (2 hunks)
  • webknossos-datastore/conf/standalone-datastore.conf (0 hunks)
💤 Files with no reviewable changes (2)
  • app/models/dataset/DataStore.scala
  • webknossos-datastore/conf/standalone-datastore.conf
🧰 Additional context used
🧬 Code graph analysis (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSRemoteWebknossosClient.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/rpc/RPCRequest.scala (1)
  • patchJson (232-236)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: backend-tests
  • GitHub Check: frontend-tests
  • GitHub Check: build-smoketest-push
🔇 Additional comments (4)
conf/application.conf (1)

99-99: LGTM! Helpful clarification added.

The inline comment clearly documents that storage scanning is controlled by the PostgreSQL reportUsedStorageEnabled flag, which aligns with the PR's objective of moving this configuration to the database.

app/controllers/WKRemoteDataStoreController.scala (1)

156-164: LGTM! Simplified status update logic.

The removal of the updateReportUsedStorageEnabledByName call correctly implements the PR objective: the datastore status update no longer overwrites the database value on every ping. The reportUsedStorageEnabled flag is now managed exclusively through direct database updates.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSRemoteWebknossosClient.scala (2)

78-81: Patch payload correctly updated.

The status report now sends only the essential fields (ok and url), which aligns with the simplified DataStoreStatus type and the controller's streamlined handling logic.


31-34: Verification complete: DataStoreStatus removal is consistent and correct.

The verification confirms that reportUsedStorageEnabled has been successfully removed from DataStoreStatus in the webknossos-datastore module with no breaking references. The field is properly retained in the main application's DataStore model (app/models/dataset/DataStore.scala), which aligns with the PR objective of managing this flag exclusively in the database. The DataStoreStatus constructor at line 81 uses only the simplified 2-field structure (ok, url), confirming the change is complete.

@fm3 fm3 merged commit c7da64a into master Oct 28, 2025
5 checks passed
@fm3 fm3 deleted the measure-storage-db-only branch October 28, 2025 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants