Skip to content

Choose between legacy/modern prefixes via prefix_style config (and trivial refactors)#276

Merged
nateware merged 14 commits intonateware:masterfrom
vizlabs:default_prefix
Dec 6, 2025
Merged

Choose between legacy/modern prefixes via prefix_style config (and trivial refactors)#276
nateware merged 14 commits intonateware:masterfrom
vizlabs:default_prefix

Conversation

@matthewhively
Copy link
Copy Markdown
Contributor

This resolves the scary upgrade messaging. No one is forced to upgrade, but it is recommended.
Now instead of configuring on a class by class basis whether they should remain with a legacy prefix, you would configure all together whether this install of Redis::Objects will remain using legacy prefixes, or has transitioned to modern prefixes.
This allows the engineer to upgrade to version 2.0 without needing to make code changes to accomodate the new prefixes.
Spec tests in redis_key_naming_spec.rb were updated to follow the new configuration.

Also included:

  • spec helper now requires active_support durations (not sure how tests were passing without this)
  • Use nested contexts within the redis_key_naming_spec.rb
  • Cleaned up some should assertions that didn't make any sense
  • Removed redundant REDIS_HOST for each of the tests within redis_key_naming_spec.rb

(Trivial refactors are shared with other branches)

@nateware
Copy link
Copy Markdown
Owner

nateware commented Dec 5, 2025

Hi Matthew, thanks for the PR for redis-objects key naming. Overall I like the approach.

I noticed you submitted 3 PR's that seem to overlap. Should I just accept the latest one? Or are all 3 needed? Thanks.

@matthewhively
Copy link
Copy Markdown
Contributor Author

Hi @nateware I submitted 3 PRs because I wanted to keep the changes grouped in some logical way. That way each could be approved or rejected independently depending on your review and feedback.
There were some common adjustments that served as a baseline for all 3 branches (and thus are included in all 3)

This PR restricts itself to the changes directly related to Redis::Objects.prefix_style
The other 2 deal specifically with:

  • migrate_redis_legacy_keys
  • comments & the readme

Ideally all 3 would be merged

@nateware nateware merged commit 8712fdb into nateware:master Dec 6, 2025
@nateware
Copy link
Copy Markdown
Owner

nateware commented Dec 6, 2025

HI @matthewhively I merged all 3 PRs and resolved one conflict. I believe I resolved it correctly. However, the new Nested tests are failing now (the ones in spec/redis_key_naming_spec.rb). Can you pull from master and see whether you can help resolve it? Thanks in advance.

@matthewhively
Copy link
Copy Markdown
Contributor Author

matthewhively commented Dec 7, 2025

Hey @nateware
Using my internal build where I had all 3 of my branches merged and comparing to your new updated master, I see the following differences:
Screenshot 2025-12-06 at 10 27 49 PM

Though that (probably) does not explain the failing tests you mentioned.

@matthewhively
Copy link
Copy Markdown
Contributor Author

@nateware well... apparently adding the deltas shown in the screenshot to my local copy of master resolved the spec error, though I have no idea why.
The error I saw on your master is:

Bacon::Error: 0.==(1) failed
	spec/redis_key_naming_spec.rb:411:in `block (3 levels) in <top (required)>': Redis key prefix naming compatibility - supports a method to migrate legacy key names

And line 411 is: obj.redis_counter.to_i.should == 1

So, apparently when scanning for the default number of 10 that it fails, but it works for 1000.
185 keys are created for this test. And 191 total keys in redis
It appears that for an unexplained reason, if all of the keys are not returned in a single loop iteration, then this test fails.


... More digging ...

Turning on verbose logging

[redis-objects] Migrating keys from 'upgrade_test' prefix to 'nested__upgrade_test'
[redis-objects] cursor: '255' #keys 184
[redis-objects] Rename 'upgrade_test:0:redis_counter', 'nested__upgrade_test:0:redis_counter'
[redis-objects] Rename 'upgrade_test:0:redis_hash', 'nested__upgrade_test:0:redis_hash'
[redis-objects] Rename 'upgrade_test:0:redis_list', 'nested__upgrade_test:0:redis_list'
...
[redis-objects] Rename 'upgrade_test::global_counter', 'nested__upgrade_test::global_counter'
[redis-objects] Rename 'upgrade_test::global_hash_key', 'nested__upgrade_test::global_hash_key'
[redis-objects] Rename 'upgrade_test::global_set', 'nested__upgrade_test::global_set'
[redis-objects] Rename 'upgrade_test::global_sorted_set', 'nested__upgrade_test::global_sorted_set'
[redis-objects] Rename 'upgrade_test::global_value', 'nested__upgrade_test::global_value'
[redis-objects] cursor: '0' #keys 1
[redis-objects] Rename 'upgrade_test:9:redis_counter', 'upgrade_test:9:redis_counter'
[redis-objects] Migrated 185 total number of redis keys

So... thats odd the key found in a subsequent loop were not renamed properly. That would certainly explain the failure... but why?


... more digging ...

Somehow during multiple loops self.redis_prefix reverts back to default.
Oh... interesting the ensure block is firing on every iteration of the loop. Thats not what I wanted.

So the fix is:
Screenshot 2025-12-06 at 11 28 23 PM

@matthewhively matthewhively mentioned this pull request Dec 7, 2025
@matthewhively
Copy link
Copy Markdown
Contributor Author

Well, its definitely a good thing I dug in to figure out why the test was failing, because otherwise the migration wouldn't have been working properly in real life scenarios.. Patch PR is up.

@nateware
Copy link
Copy Markdown
Owner

nateware commented Dec 8, 2025

Nice - thanks for the deep dive @matthewhively! I merged that other PR and it seems like tests are all passing now.

To verify, the new default behavior is one that is backwards compatible, is that correct? So somebody would elect to use the new full prefixes if needed, is that right?

@matthewhively
Copy link
Copy Markdown
Contributor Author

@nateware yes, that was my intention.
In theory in a future release the default behavior could be updated to modern and then a far future release could drop support for the deprecated legacy prefixes entirely.

@nateware
Copy link
Copy Markdown
Owner

nateware commented Dec 9, 2025 via email

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.

2 participants