search

twingly / twingly-amqp

:bus: Ruby gem for RabbitMQ subscribing and publishing
0 stars 0 forks source link

Should we cache here or in consumers? #7

Closed walro closed 9 years ago

walro commented 9 years ago

From HipChat:

[10:26] Robin Wallin: hmm ska cache:n ligga i twingly-amqp ? är tanken att flera olika konsumenter kan dela cache då eller?
        känns lite konsigt att ha den där imo 
[10:28] Mattias Roback: det var mest för att slippa gå igenom alla urler innan man pingar för att se om de är cachade
[10:29] Robin Wallin: ok, fast ändå lite märklig placering av ansvar
        hade nog tagit den smällen
[10:30] Mattias Roback: det var mest för att få ihop det med remora utan att göra alltför många ändringar i koden (remora hade cache:n i pinger-klassen innan)
        öppna en issue :)
[10:31] Robin Wallin: tänkte känna av först, om ingen håller med mig kändes issue onödigt :)
[10:31] Patrik Ragnarsson: jag tror jag håller med dig @RobinWallin :) men har inte riktigt koll på vad som gjorts än
[10:31] Mattias Roback: Jag tänkte samma sak igår, men då hade jag ju redan lagt cache:n i ping :)
[10:32] Johan Eckerström: Kan tänka mig att skissa lite på en whiteboard sen, samt jämföra lite med hur t.ex rack och diverse http-klienter fungerar, om man kallar den pingcache istället för urlcache känns det rimligt att man kan injecta en cache som används i ett gem.
dentarg commented 9 years ago

Kan man verkligen säga att cache:en ligger i twingly-amqp nu? Kan man inte se det som att twingly-amqp har "caching" som en feature?

En anledning till att vi ville skapa detta gem var ju att vi gör samma sak i många projekt: vi pingar och vi cache:ar

Men det är möjligt att vi kanske ska ha ett cache-gem också, istället för att ha det i detta gem :)

walro commented 9 years ago

Jag tycker det är märkligt att ett gem som heter "twingly-amqp" har möjlighet att hantera cachning av url:er. Det blir ännu märkligare när vi lägger till fler metoder här (just nu finns bara ping).

walro commented 9 years ago

Imo ska man ge den en lista med url:er att pinga, that's it. Klienten får ha listat ut innan vad som ska pingas eller inte.

dentarg commented 9 years ago

Jo... som @jage skrev kanske är en "pingcache" är mer rätt?

och den kanske ska användas när man pingar? ping(url, cache: true)?

dentarg commented 9 years ago

Jo, jag håller nog med, vi ska nog inte cache:a pings... som du säger, den som ping:ar borde cache:a URL:er, sedan ping:a det som ska pingas. Sedan borde det som tar emot pings ha en cache eller något för att ta bort onödigt arbete. Typ?

walro commented 9 years ago

Jo, jag håller nog med, vi ska nog inte cache:a pings... som du säger, den som ping:ar borde cache:a URL:er, sedan ping:a det som ska pingas. Sedan borde det som tar emot pings ha en cache eller något för att ta bort onödigt arbete. Typ?

Mm, känns renast, men jag förstår att vi kan skjuta på det. Detta fungerar ju bra, rent vad-kunderna-får-ut-mässigt. :)

jage commented 9 years ago

Tycker det här hänger ihop med ansvaret på en högre nivå (norrstrom, chapman etc), att varje tjänst ska ha ett ansvar. Att man har ett bibliotek som har lite extra funktioner tycker jag inte är något problem i sig, men om det lockar till att bryta mot tjänste-uppdelningen bör man nog ta bort funktionen.

Jag håller med om att vi nog bör cache:a så sent som möjligt och dra nytta av en delad cache. Dock kommer vi tappa en del metrics, jag tycker det är lite intressant att se när Remora hittar nya URLer etc.

jage commented 9 years ago

Looks like we still want the ability to cache in the producers (pingers). Question answered?

walro commented 9 years ago

Looks like we still want the ability to cache in the producers (pingers). Question answered?

Not sure what you mean. Do you mean that we should not touch this library (i.e. let it accept an cache) and close this issue?

jage commented 9 years ago

Do you mean that we should not touch this library (i.e. let it accept an cache) and close this issue?

Yes. Norrstrom/task isn't an option right now so we need to a cache in our producers. We could remove the UrlCache-API in here and let each application implement it, but for me it really don't matter. I think this is very convenient to just pass Twingly::UrlCache to Ping.

walro commented 9 years ago

We could remove the UrlCache-API in here and let each application implement it, but for me it really don't matter

It was this part I was interested in. Sure, we can keep it as-is. Closing.