Skip to content

NoSuchCacheManagerFailureAnalyzer #13916

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

NoSuchCacheManagerFailureAnalyzer #13916

wants to merge 4 commits into from

Conversation

nosan
Copy link
Contributor

@nosan nosan commented Jul 26, 2018

see gh-13348

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 26, 2018
@nosan nosan force-pushed the gh-13348 branch 2 times, most recently from 8d59b76 to 51fe5da Compare July 26, 2018 19:51
snicoll
snicoll requested changes Jul 27, 2018
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @nosan

I've made a few suggestions. I also wonder if this isn't a special case of NoSuchBeanDefinitionFailureAnalyzer. If a CacheManager could not be auto-configured, I'd like to see the auto-configurations pieces that couldn't apply and the conditions that weren't met.

I certainly wouldn't want to copy/paste that logic and I think it'd be worth researching.

Thoughts?

...igure/src/main/java/org/springframework/boot/autoconfigure/cache/CacheAutoConfiguration.java Outdated
* @see EnableCaching
* @since 1.3.0
Copy link
Member

Choose a reason for hiding this comment

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

please refrain from doing unrelated changes.

...igure/src/main/java/org/springframework/boot/autoconfigure/cache/CacheAutoConfiguration.java Outdated
* Exception thrown when {@link org.springframework.cache.CacheManager} implementation
* are not specified.
*/
static class NoCacheManagerException extends RuntimeException {
Copy link
Member

Choose a reason for hiding this comment

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

To mirror NoSuchBeanDefinitionException, you could call that NoSuchCacheManagerException

...igure/src/main/java/org/springframework/boot/autoconfigure/cache/CacheAutoConfiguration.java Outdated
* implementations are available with no way to know which implementation should be
* used.
*/
static class NonUniqueCacheManagerException extends RuntimeException {
Copy link
Member

Choose a reason for hiding this comment

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

And this one could be NoUniqueCacheManagerException extending from the other exception (again to mirror NoUniqueBeanDefinitionException)

...rc/main/java/org/springframework/boot/autoconfigure/cache/NoCacheManagerFailureAnalyzer.java Outdated
* @author Dmytro Nosan
* @since 2.1.0
*/
class NoCacheManagerFailureAnalyzer
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to merge the FailureAnalyzer so that we only have one class (and an instance check to determine the type). This is my main motivation for having a hierarchy in the two exceptions at hand.

.../src/test/java/org/springframework/boot/autoconfigure/cache/CacheAutoConfigurationTests.java Outdated
@@ -1007,6 +1018,16 @@ public CacheResolver cacheResolver() {

}

@Configuration
static class AdditionalCacheManagerAutoConfiguration {
Copy link
Member

Choose a reason for hiding this comment

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

That's not an auto-configuration and shouldn't be named like that. It shouldn't be added as an auto-config in the test either.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jul 27, 2018
@nosan nosan force-pushed the gh-13348 branch 5 times, most recently from 2a4aa4b to fa1c719 Compare July 27, 2018 09:21
@nosan
Copy link
Contributor Author

nosan commented Jul 27, 2018 •
edited
Loading

@snicoll Thank you.
I have updated PR, hope, I understood you right. Could you please check NoSuchCacheManagerFailureAnalyzer once more? Should it be Ordered or just add it before NoSuchBeanDefinitionFailureAnalyzer?

@nosan nosan changed the title NoCacheManagerFailureAnalyzer, NonUniqueCacheManagerFailureAnalyzer NoSuchCacheManagerFailureAnalyzer Jul 27, 2018
@snicoll
Copy link
Member

snicoll commented Jul 27, 2018 •
edited
Loading

@nosan thanks for the update. I didn't mean to extend from the framework exception type but to mirror the same naming and hierarchy. The base should extend from RuntimeException and so there is no need to order anything.

Sorry if that wasn't clear.

@nosan
Copy link
Contributor Author

nosan commented Jul 27, 2018 •
edited
Loading

@snicoll Frankly speaking, I really like these two analyzers NoSuchBeanDefinitionFailureAnalyzer and NoUniqueBeanDefinitionFailureAnalyzer and I'd like to reuse them as much as possible, that's why I'd prefer to extend NoUniqueBeanDefinitionException and NoUniqueBeanDefinitionException accordingly and provide NoSuchCacheManagerFailureAnalyzer for a spring.cache.type=generic case only.

Thoughts?

@nosan
Copy link
Contributor Author

nosan commented Jul 28, 2018

I need to revise this PR.

@nosan
Copy link
Contributor Author

nosan commented Jul 31, 2018

@snicoll I've updated this PR. Please take a look :)
Thanks in advance.

@snicoll snicoll self-requested a review August 1, 2018 06:54
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Aug 1, 2018
@snicoll snicoll mentioned this pull request Aug 1, 2018
Closed
@nosan nosan force-pushed the gh-13348 branch 2 times, most recently from 0374b2b to a3dc14c Compare August 1, 2018 14:15
@nosan nosan force-pushed the gh-13348 branch 2 times, most recently from 4a1a22b to 742ba6e Compare August 21, 2018 13:57
@nosan
Copy link
Contributor Author

nosan commented Dec 3, 2018 •
edited
Loading

@snicoll

Sorry for the long delay, I was a bit busy :(

I've updated this PR and fix all conflicts.

@snicoll
Copy link
Member

snicoll commented Dec 4, 2018 •
edited
Loading

@nosan no need to apologize, I am the one who didn't get a chance to have a deeper look at it yet. Thanks for rebasing!

@nosan nosan force-pushed the gh-13348 branch 2 times, most recently from beb5766 to f7039a6 Compare December 10, 2018 13:49
@nosan nosan force-pushed the gh-13348 branch 3 times, most recently from 84a5fa1 to c11e571 Compare February 13, 2019 18:13
@snicoll
Copy link
Member

snicoll commented Feb 20, 2019

Thanks for your patience @nosan. I am not keen to bring this amount of complexity for the task at hand. I've created an issue in Framework to improve the exception that is throw in such cases so that we can reuse the existing analyzers or very easily tune a new one for our purposes. Thanks for the PR, in any case.

@snicoll snicoll closed this Feb 20, 2019
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed type: enhancement A general enhancement labels Feb 20, 2019
@nosan nosan deleted the gh-13348 branch February 20, 2019 13:24
@nosan
Copy link
Contributor Author

nosan commented Feb 20, 2019

@snicoll fair enough, this PR contained a lot of changes.

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

@snicoll snicoll Awaiting requested review from snicoll

Assignees
No one assigned
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nosan @snicoll @spring-projects-issues