Skip to content

Comments

Add STALE command flag to SCRIPT-EXISTS, SCRIPT-SHOW and SCRIPT-FLUSH#2419

Merged
enjoy-binbin merged 2 commits intovalkey-io:unstablefrom
enjoy-binbin:script
Aug 6, 2025
Merged

Add STALE command flag to SCRIPT-EXISTS, SCRIPT-SHOW and SCRIPT-FLUSH#2419
enjoy-binbin merged 2 commits intovalkey-io:unstablefrom
enjoy-binbin:script

Conversation

@enjoy-binbin
Copy link
Member

@enjoy-binbin enjoy-binbin commented Aug 4, 2025

We marked SCRIPT-LOAD/EVAL* with STALE in 7eadc5e,
it is odd that we can load but won't be able to exists or show it.
Also it is technically ok since these commands doesn't relate directly
to the server's dataset.

Also since now we don't have the script replication, flush also seems
safe to add the flag.

We marked SCRIPT-LOAD/EVAL* with STALE in 7eadc5e,
it is odd that we can load but won't be able to exists or show it.
Also it is technically ok since these commands doesn't relate directly
to the server's dataset.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin
Copy link
Member Author

I'm struggling with script-flush, it seem we can do the same thing (or can't)

Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Yeah, I don't see any reason why not to show the script contents while the server is stale.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Seems OK. Not sure why it's useful though. According to this comment, only a few commands are accepted during STALE:

 * CMD_STALE:       Allow the command while a replica has stale data but is not
 *                  allowed to serve this data. Normally no command is accepted
 *                  in this condition but just a few.

@enjoy-binbin
Copy link
Member Author

The comment is a bit outdated i guess, searing "STALE" in json files we can see we allow 140+ commands to run in a stale server. I guess it originally meant to say a few "keyspace or data" commands

@enjoy-binbin
Copy link
Member Author

enjoy-binbin commented Aug 5, 2025

what about script-flush? i am going to add STATE to it as well, i don't see the problem here since now we don't have the script replication, so even the user flush it all, it is his choice?

Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin enjoy-binbin changed the title Add STALE command flag to SCRIPT-EXISTS and SCRIPT-SHOW Add STALE command flag to SCRIPT-EXISTS, SCRIPT-SHOW and SCRIPT-FLUSH Aug 5, 2025
@codecov
Copy link

codecov bot commented Aug 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.42%. Comparing base (3b12132) to head (a680c17).
⚠️ Report is 18 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2419      +/-   ##
============================================
- Coverage     71.51%   71.42%   -0.10%     
============================================
  Files           123      123              
  Lines         67454    67487      +33     
============================================
- Hits          48239    48201      -38     
- Misses        19215    19286      +71     
Files with missing lines Coverage Δ
src/commands.def 100.00% <ø> (ø)
src/server.h 100.00% <ø> (ø)

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@enjoy-binbin enjoy-binbin added the release-notes This issue should get a line item in the release notes label Aug 6, 2025
@enjoy-binbin
Copy link
Member Author

I see we already have three votes with the new change. This isn't particularly matter and it is harmless, so i don't think it will be a major decision and i am going to merge it.

@enjoy-binbin enjoy-binbin merged commit db11e41 into valkey-io:unstable Aug 6, 2025
52 of 53 checks passed
@enjoy-binbin enjoy-binbin deleted the script branch August 6, 2025 02:37
allenss-amazon pushed a commit to allenss-amazon/valkey-core that referenced this pull request Aug 19, 2025
…valkey-io#2419)

We marked SCRIPT-LOAD/EVAL* with STALE in
7eadc5e,
it is odd that we can load but won't be able to exists or show it.
Also it is technically ok since these commands doesn't relate directly
to the server's dataset.

Also since now we don't have the script replication, flush also seems
safe to add the flag.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes This issue should get a line item in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants