Added implicit PRUNE mode warning & Handling invalid data-storage-mode file #8369

lucassaldanha · 2024-06-12T05:05:52Z

PR Description

Handling error when data-storage-mode.txt file is invalid/empty/corrupt, including a manual step the user can execute to fix it.
Added deprecation warning for implicit PRUNE data storage mode.

WARNING
In the next release, Teku will stop supporting implicit PRUNE database storage mode configuration. If you want to keep using PRUNE mode, you must update your config file or CLI options with --data-storage-mode=PRUNE

Fixed Issue(s)

fixes #8357

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

zilm13 · 2024-06-12T18:38:49Z

CHANGELOG.md

@@ -5,6 +5,7 @@
 - Next release will introduce a breaking change to Teku's metrics. This is due to some metrics changing names after a library upgrade.
 We recommend all users of the `Teku - Detailed` dashboard to upgrade to version [Revision 12](https://grafana.com/api/dashboards/16737/revisions/12/download)  
 as soon as possible. Documentation with all metrics that have been renamed will be provided.
+- Next release will stop supporting implicit PRUNE database storage mode configuration. If you want to keep using PRUNE mode, you must update your config file or CLI options with --data-storage-mode=PRUNE.


It's difficult to understand. Maybe focus on default behavior changing from PRUNE to MINIMAL unless explicitely configured something else ?

i think this is specifically focussed on what we need users to do, im not sure we can make it easier to understand without saying

From the next release, you will need to explicitly set `--data-storage-mode=(prune|archive)` unless you're using minimal data-storage-mode (which is default behaviour)

I like your version more. Some users don't even know which mode they are using and what is the difference

zilm13 · 2024-06-12T18:41:36Z

infrastructure/logging/src/main/java/tech/pegasys/teku/infrastructure/logging/StatusLogger.java

+        print(
+            "Prune mode being used as default without a explicit --data-storage-mode option. This will NOT be "
+                + "supported in future Teku versions. Please add --data-storage-mode=prune to your CLI arguments"
+                + " or config file.",


.. if you want to keep it"?

zilm13 · 2024-06-12T18:45:21Z

storage/src/main/java/tech/pegasys/teku/storage/server/StorageConfiguration.java

-                dataDirLayout.getBeaconDataDirectory().toFile().exists(),
-                getStorageModeFromPersistedDatabase(dataDirLayout),
-                dataStorageMode);
+                beaconDataDirectory.toFile().exists(), storageModeFromStoredFile, dataStorageMode);
      } else {
        if (dataStorageMode.equals(NOT_SET)) {
          dataStorageMode = PRUNE;


I don't understand this, it defaults to PRUNE

If next one answer is "YES", this one is clear. Do we need to print warn here too?

To be honest, I think I can clear this up... I don't think there is a code path where dataConfig will ever be null. So this else should never happen in production.

zilm13 · 2024-06-12T18:52:42Z

storage/src/main/java/tech/pegasys/teku/storage/server/StorageConfiguration.java

+
+    if (isExistingStore) {
+      STATUS_LOG.warnUsageOfImplicitPruneDataStorageMode();
+      return PRUNE;


so we wll change this behavior in another release, right?

Correct: #8371

zilm13 · 2024-06-12T18:58:26Z

storage/src/main/java/tech/pegasys/teku/storage/server/VersionedDatabaseFactory.java

      }
-    } catch (IOException e) {
-      LOG.error("Failed to read storage mode file", e);
    }

    return maybeConfiguredStorageMode.orElse(StateStorageMode.DEFAULT_MODE);


method name is getStateStorageModeFromConfigOrDisk, but we read only for config actually and warn if persisted doesn't match. Maybe rename method?

Now that I am thinking about it, we can probably get rid of this method... There is no point in warning about this anymore as we are planning to get rid of this behaviour soon... Thoughts?

Yeah, we print actually used, we don't use file info here, so it makes sense (to clean it)

zilm13 · 2024-06-12T18:59:00Z

storage/src/main/java/tech/pegasys/teku/storage/server/VersionedDatabaseFactory.java

+    if (maybeStorageModeFromFile.isPresent() && maybeConfiguredStorageMode.isPresent()) {
+      if (!maybeStorageModeFromFile.get().equals(maybeConfiguredStorageMode.get())) {
+        LOG.info(
+            "Storage mode that was persisted differs from the command specified option; file: {}, CLI: {}",


It would be good to see here which one will be actually used. And maybe it should be warn?

We have another log line that says what data storage mode is being used. Not sure if there is value in adding it here as well.

Right, this one is called on init, another one later will log actual

…ync writer)

rolfyone

LGTM check tests pls

lucassaldanha changed the title ~~Handling invalid data-storage-mode file~~ Add implicit PRUNE mode warning & Handling invalid data-storage-mode file Jun 12, 2024

lucassaldanha changed the title ~~Add implicit PRUNE mode warning & Handling invalid data-storage-mode file~~ Added implicit PRUNE mode warning & Handling invalid data-storage-mode file Jun 12, 2024

lucassaldanha mentioned this pull request Jun 12, 2024

Stop supporting implicit PRUNE data storage mode #8371

Open

3 tasks

zilm13 reviewed Jun 12, 2024

View reviewed changes

lucassaldanha added 6 commits June 18, 2024 11:31

Handling errors when reading database storage mode file (and using as…

24593f1

…ync writer)

Handling errors when reading database storage mode file (and using as…

f72e979

…ync writer)

Add warning for implicit prune mode as default

eb4ba8c

changelog

3e5755b

Assemble error (finals)

ddbc0aa

PR comments

130f275

lucassaldanha force-pushed the teku-8357 branch from 7899533 to 130f275 Compare June 17, 2024 23:33

rolfyone approved these changes Jun 18, 2024

View reviewed changes

Restore dataConfig == null path (for tests)

aa49cd5

lucassaldanha merged commit 437da16 into Consensys:master Jun 18, 2024
16 checks passed

bgravenorst mentioned this pull request Jul 16, 2024

Remove any support for PRUNE mode Consensys/doc.teku#588

Closed

lucassaldanha deleted the teku-8357 branch July 31, 2024 23:36

joaniefromtheblock mentioned this pull request Aug 27, 2024

Remove references to Prune Consensys/doc.teku#595

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added implicit PRUNE mode warning & Handling invalid data-storage-mode file #8369

Added implicit PRUNE mode warning & Handling invalid data-storage-mode file #8369

lucassaldanha commented Jun 12, 2024 •

edited

Loading

zilm13 Jun 12, 2024

rolfyone Jun 12, 2024

zilm13 Jun 12, 2024

zilm13 Jun 12, 2024

zilm13 Jun 12, 2024

zilm13 Jun 12, 2024

lucassaldanha Jun 12, 2024

zilm13 Jun 12, 2024

lucassaldanha Jun 12, 2024 •

edited

Loading

zilm13 Jun 12, 2024

lucassaldanha Jun 12, 2024

zilm13 Jun 12, 2024

zilm13 Jun 12, 2024

lucassaldanha Jun 12, 2024

zilm13 Jun 12, 2024

rolfyone left a comment

Added implicit PRUNE mode warning & Handling invalid data-storage-mode file #8369

Added implicit PRUNE mode warning & Handling invalid data-storage-mode file #8369

Conversation

lucassaldanha commented Jun 12, 2024 • edited Loading

PR Description

Fixed Issue(s)

Documentation

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucassaldanha Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolfyone left a comment

Choose a reason for hiding this comment

lucassaldanha commented Jun 12, 2024 •

edited

Loading

lucassaldanha Jun 12, 2024 •

edited

Loading