Skip to content
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

Add method Cell#cached? #478

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PikachuEXE
Copy link
Contributor

This is for logging cache hit rate for specific cells
Looking at caching server hit rate is useless and also inaccurate since rails 5.2's cache versioning

Copy link
Member

@seuros seuros left a comment

Choose a reason for hiding this comment

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

Great addition.
Can you fix the build ? It failing in one spec.

@PikachuEXE
Copy link
Contributor Author

Spec fixed

@PikachuEXE
Copy link
Contributor Author

Hello any more change required?

@PikachuEXE
Copy link
Contributor Author

LOL almost forgot about this PR
Can this be merged?

@PikachuEXE
Copy link
Contributor Author

Yo?

@PikachuEXE
Copy link
Contributor Author

Hello?

@PikachuEXE
Copy link
Contributor Author

@seuros :) ?
It's been sitting here approved for over 7 months
I think this boy deserves some attention 👋

Copy link
Member

@apotonick apotonick left a comment

Choose a reason for hiding this comment

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

Sorry, this can't be approved until we remove the redundancy from Caching#render_state, there are two really complex identical lines. Also, I'm a bit nervous about cache_store.exist?(key, options) since some people might use e.g. a Hash cache store? Why don't you just fetch the content the way it's done in render_state to see if it's cached?

def cached?(state=:show, *args)
return false unless cache?(state, *args)

key = self.class.state_cache_key(state, self.class.version_procs[state].(self, *args))
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this is repeating code from this file?

Copy link
Member

Choose a reason for hiding this comment

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

Line 70-71 are repeating already existing code. This is not a good idea, we should extract it to one method if this gets approved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Method extracted

end.new

def with_cache_cleanup
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this method, I wonder? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no after(:each) like rspec
So add this for cleanup after each test

key = self.class.state_cache_key(state, self.class.version_procs[state].(self, *args))
options = self.class.cache_options[state].(self, *args)

cache_store.exist?(key, options)
Copy link
Member

Choose a reason for hiding this comment

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

Where does #exist? come from? Is that the official name in cache stores, or should there rather be key? for Hash-compat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apotonick
Copy link
Member

Do you have any example on where this method is used in your apps? 🍻

@PikachuEXE
Copy link
Contributor Author

I mainly use this method for

  • Tracking (whether transactions are having main page content cell cached)
  • Page content "warming" (Keep important page content cells cached by running in bg workers)

@PikachuEXE
Copy link
Contributor Author

Build failed in 2.2 due to
image

No test case is failing

@PikachuEXE
Copy link
Contributor Author

Build for ruby 2.2 fixed
But I guess that change should be applied on master instead

@PikachuEXE
Copy link
Contributor Author

Anything else I should respond to or update?

@PikachuEXE
Copy link
Contributor Author

Hello again :3 ?

@apotonick
Copy link
Member

apotonick commented Aug 9, 2019

Thanks @PikachuEXE for your contributions! ❤️ We are still mulling over the decision whether or not this addition is really needed in the core code. It's a helpful method, but every method added is a method you need to maintain, test, and deprecate at some point. The more public API an object has, the worse it gets (see ActiveRecord for reference).

One idea I have is we could have a cells-cached gem and still merge your internal refactorings in cells core. With the download statistics we can then analyze how much this addition is being used - I am guessing, it won't be used a lot (#nojudging ;).

I apologize if that feels "unfair" but I'm planning a very slim Cells 5 and the more we add now, the more we have to deprecate, and in all fairness, there are more pressing issues than a caching tester, so please bear with me and think about my suggestions again.

@PikachuEXE
Copy link
Contributor Author

I am totally fine with having an extension gem instead of merging it into core.
Tell me what I can/should do ;)

@PikachuEXE
Copy link
Contributor Author

Also to let people know about your plan of releasing slim gem(s)
You might want to consider putting a PULL_REQUEST_TEMPLATE to let other guys know when they submit PR.

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.

3 participants