Skip to content

Deprecate snap server experimental option and add stable option #8512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pullurib
Copy link
Contributor

@pullurib pullurib commented Apr 3, 2025

PR description

Deprecate --Xsnapsync-server-enabled and add --snapsync-server-enabled instead.

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • spotless: ./gradlew spotlessApply
  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@pullurib pullurib requested a review from garyschulte April 3, 2025 05:12
@macfarla macfarla added the doc-change-required Indicates an issue or PR that requires doc to be updated label Apr 7, 2025
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Apr 10, 2025
@macfarla
Copy link
Contributor

@garyschulte are we ready to remove the --X from snap server?

@@ -445,7 +457,7 @@ public SynchronizerConfiguration.Builder toDomainObject() {
.trienodeCountPerRequest(snapsyncTrieNodeCountPerRequest)
.localFlatAccountCountToHealPerRequest(snapsyncFlatAccountHealedCountPerRequest)
.localFlatStorageCountToHealPerRequest(snapsyncFlatStorageHealedCountPerRequest)
.isSnapServerEnabled(snapsyncServerEnabled)
.isSnapServerEnabled(snapsyncServerEnabled || snapsyncServerEnabledExperimental)
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't the flow we typically use when we promote something out of experimental. Typically we just alias the -X and after a quarter or so just remove the flag. We can add a deprecation message without adding an additional flag .

That said, this isn't a horrible pattern. At a minimum there needs to be a CHANGELOG.md entry for this PR, and some text in the upcoming deprecation body that references it.

@pullurib pullurib force-pushed the snapserver-option branch from 6db1bf9 to f53300f Compare May 22, 2025 21:13
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

can you also update the log message in the startup summary to remove Experimental from:

# Experimental Snap Sync server enabled

@pullurib pullurib force-pushed the snapserver-option branch from 154f4ce to 048df22 Compare May 23, 2025 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants