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

Related to Issue #200, slight code clean-up. #243

Merged
merged 1 commit into from Feb 27, 2012

Conversation

Projects
None yet
2 participants
@leehambley
Contributor

leehambley commented Feb 27, 2012

This is a trivial pull request, it (naturally) still passes all tests, but is intended to allow me to more easily extend the SlugGenerator class, and borrow the behaviour which helps me writing my own implementation (#wildcard, #column, #separator, etc) whilst extending the parts which might need to be modified.

I'm planning to take the short route, to implementing my own Redis backed option, and would like to see this come upstream so that I might determine the best places to hook/override behaviour.

norman added a commit that referenced this pull request Feb 27, 2012

Merge pull request #243 from leehambley/master
Related to Issue #200, slight code clean-up.

@norman norman merged commit 9ca0cf2 into norman:master Feb 27, 2012

@norman

This comment has been minimized.

Show comment
Hide comment
@norman

norman Feb 27, 2012

Owner

Pulled. A few things to keep in mind:

  • I won't pull anything that adds even an optional Redis dependency. If you want to implement for your own use, cool - but I'm just giving you a heads up so you don't waste time. Using Redis to do something like do unique sequencing in an RDBMS is pointless and unnecessary.
  • Also keep in mind, that the sequencing feature tries to maintain a low sequence number per-slug which is why regular Postgres sequences or MySQL auto-increment columns aren't currently used for that.
  • The API will probably also change significantly here in 4.1, because I plan to modify the slug sequencing work like an optional extension. For example, some people are fine with adding a number to the end of non-unique slugs, but other people have asked me for the ability to try generating a different slug when the default one is not unique. I plan to change the API to make this possible.

So if you set up your own Redis sequencer, as a separate library right now, my changes in 4.1 will very likely break it, though it's also very likely that I'll expose a public API that would allow you to implement your own sequencing strategy (using for example, Redis, Hadoop, or a cluster of Cray supercomputers), and that would then be a stable API that you could count on me not breaking.

Owner

norman commented Feb 27, 2012

Pulled. A few things to keep in mind:

  • I won't pull anything that adds even an optional Redis dependency. If you want to implement for your own use, cool - but I'm just giving you a heads up so you don't waste time. Using Redis to do something like do unique sequencing in an RDBMS is pointless and unnecessary.
  • Also keep in mind, that the sequencing feature tries to maintain a low sequence number per-slug which is why regular Postgres sequences or MySQL auto-increment columns aren't currently used for that.
  • The API will probably also change significantly here in 4.1, because I plan to modify the slug sequencing work like an optional extension. For example, some people are fine with adding a number to the end of non-unique slugs, but other people have asked me for the ability to try generating a different slug when the default one is not unique. I plan to change the API to make this possible.

So if you set up your own Redis sequencer, as a separate library right now, my changes in 4.1 will very likely break it, though it's also very likely that I'll expose a public API that would allow you to implement your own sequencing strategy (using for example, Redis, Hadoop, or a cluster of Cray supercomputers), and that would then be a stable API that you could count on me not breaking.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Feb 27, 2012

Contributor

Pulled.

Thanks! Great response time.

I won't pull anything that adds even an optional Redis dependency.

Absolutely acknowledged, I have a special case here, that the table has tens of millions of records, and the read locks kill me. Also when you have a Redis hammer, everything looks like a nail, that really ought to stay my problem, and no go upstream!

Also keep in mind, that the sequencing feature tries to maintain a low sequence number per-slug...

I hadn't thought about that, something I need to amend my case to check for, that is #{normalized}-#{sequence} not #{model_class}-#{sequence}, my Redis implementation is a model-wise sequence, which is clearly wrong now that you mention it.

The API will probably also change significantly here in 4.1, because I plan to modify the slug sequencing work like an optional extension.

No problem for me, I won't release my stuff publicly right now, I have enough open source that requires maintenance, and regarding 4.1, that's the contract we all opt-into when using semver, or similar.

it's also very likely that I'll expose a public API that would allow you to implement your own sequencing strategy.

It might be worth pointing out that the name SlugGenerator is a little unclear, I expected this class would be responsible for generating the actual friendly ID, the task currently handled by Slugged#normalize_friendly_id, perhaps the class should be renamed when you change the API? We've been using the term slug sequencer or unique sequencer quite a bit when discussing this… anyway, just a thought. 😄

Contributor

leehambley commented Feb 27, 2012

Pulled.

Thanks! Great response time.

I won't pull anything that adds even an optional Redis dependency.

Absolutely acknowledged, I have a special case here, that the table has tens of millions of records, and the read locks kill me. Also when you have a Redis hammer, everything looks like a nail, that really ought to stay my problem, and no go upstream!

Also keep in mind, that the sequencing feature tries to maintain a low sequence number per-slug...

I hadn't thought about that, something I need to amend my case to check for, that is #{normalized}-#{sequence} not #{model_class}-#{sequence}, my Redis implementation is a model-wise sequence, which is clearly wrong now that you mention it.

The API will probably also change significantly here in 4.1, because I plan to modify the slug sequencing work like an optional extension.

No problem for me, I won't release my stuff publicly right now, I have enough open source that requires maintenance, and regarding 4.1, that's the contract we all opt-into when using semver, or similar.

it's also very likely that I'll expose a public API that would allow you to implement your own sequencing strategy.

It might be worth pointing out that the name SlugGenerator is a little unclear, I expected this class would be responsible for generating the actual friendly ID, the task currently handled by Slugged#normalize_friendly_id, perhaps the class should be renamed when you change the API? We've been using the term slug sequencer or unique sequencer quite a bit when discussing this… anyway, just a thought. 😄

@norman

This comment has been minimized.

Show comment
Hide comment
@norman

norman Feb 27, 2012

Owner

I have a special case here, that the table has tens of millions of records, and the read locks kill me.

Cool, that's at least an interesting problem to have. :) Also an even better argument for making a real API for this so that people with special needs like you have a reasonably straightforward way to implement special-case solutions.

Still, there's no excuse for FriendlyId not being able to handle this without concurrency problems out of the box, even if it's slow. You should only have to optimize for performance in your case, not for correctness.

It might be worth pointing out that the name SlugGenerator is a little unclear

Totally agree, it should be changed.

Owner

norman commented Feb 27, 2012

I have a special case here, that the table has tens of millions of records, and the read locks kill me.

Cool, that's at least an interesting problem to have. :) Also an even better argument for making a real API for this so that people with special needs like you have a reasonably straightforward way to implement special-case solutions.

Still, there's no excuse for FriendlyId not being able to handle this without concurrency problems out of the box, even if it's slow. You should only have to optimize for performance in your case, not for correctness.

It might be worth pointing out that the name SlugGenerator is a little unclear

Totally agree, it should be changed.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Feb 27, 2012

Contributor

You should only have to optimize for performance in your case, not for correctness.

You're right - but I still think that for /most people/ who're using friendly_id to get up and running quickly with beautiful URLs, what's in the box right now is perfectly OK for most people, so something extra, and optional would be perfect (and why not a transaction locked slugs table, as previously) for those generating/importing data in background processes, which although widespread isn't exactly the most common case.

I'm quite happy with my redis implementation now, I haven't tried it in action yet - but unless I'm mistaken I've tested all but the timeout cases, testing timeouts is something I don't really want to bite off right now, and that's more for covering my ass. If you are curious (and I would genuinely appreciate your insight, if you have a moment to read through) - Including tests all inline at https://gist.github.com/c014dfcc24f80f803621, and the blog post I wrote detailing my thought process http://lee.hambley.name/2012/2/27/friendly_id-and-parallel-processes/

Contributor

leehambley commented Feb 27, 2012

You should only have to optimize for performance in your case, not for correctness.

You're right - but I still think that for /most people/ who're using friendly_id to get up and running quickly with beautiful URLs, what's in the box right now is perfectly OK for most people, so something extra, and optional would be perfect (and why not a transaction locked slugs table, as previously) for those generating/importing data in background processes, which although widespread isn't exactly the most common case.

I'm quite happy with my redis implementation now, I haven't tried it in action yet - but unless I'm mistaken I've tested all but the timeout cases, testing timeouts is something I don't really want to bite off right now, and that's more for covering my ass. If you are curious (and I would genuinely appreciate your insight, if you have a moment to read through) - Including tests all inline at https://gist.github.com/c014dfcc24f80f803621, and the blog post I wrote detailing my thought process http://lee.hambley.name/2012/2/27/friendly_id-and-parallel-processes/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment