Add cache actuator endpoint for listing and clearing caches #12216

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

joshiste wants to merge 4 commits into spring-projects:master from joshiste:caches-endpoint

Contributor

joshiste commented Feb 25, 2018

This commits adds an actuator endpoint which lists the caches per
context and cacheManager and provides a delete operation to clear the
caches. As the statistics are exposed via the metrics endpoint they are
not included

closes #2625

spring-projects-issues added the status: waiting-for-triage label

joshiste force-pushed the caches-endpoint branch 4 times, most recently from 67b1140 to 86e5b2a Compare

February 25, 2018 19:30

philwebb added type: enhancement priority: normal and removed status: waiting-for-triage labels

philwebb added this to the 2.1.0 milestone

Member

philwebb commented Feb 25, 2018

Nice, thanks.

Member

snicoll commented Feb 26, 2018

snicoll requested changes

View reviewed changes

Member

snicoll left a comment

Thanks for the PR. I think there are several areas that need discussion, see my comments.

...nfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/cache/package-info.java Outdated

    
               */

              /**

               * Auto-configuration for actuator Cache concerns.

Member

snicoll Feb 26, 2018

Not sure we need an upper case for cache.

...ator-autoconfigure/src/main/resources/META-INF/additional-spring-configuration-metadata.json Outdated

    
                    }

                  },

                  {

                    "name": "endpoints.caches.enabled",

Member

snicoll Feb 26, 2018

You don't need to write deprecation information for an endpoint that did not exist in 1.5

...spring-boot-actuator/src/main/java/org/springframework/boot/actuate/cache/CacheEndpoint.java Outdated

    
               * @author Johannes Edmeuer

               * @since 2.0.0

               */

              @Endpoint(id = "caches")

Member

snicoll Feb 26, 2018

The id should be cache IMO (if you feel strongly otherwise, you need to rename that class then).

...spring-boot-actuator/src/main/java/org/springframework/boot/actuate/cache/CacheEndpoint.java Outdated

    
              	}

              	@ReadOperation

              	public ApplicationCacheManagerBeans cacheManagerBeans() {

Member

snicoll Feb 26, 2018

cacheManagers sounds more sensible to me.

...spring-boot-actuator/src/main/java/org/springframework/boot/actuate/cache/CacheEndpoint.java Outdated

    
              	}

              	@DeleteOperation

              	public void clearCaches(@Nullable String contextId, @Nullable String cacheManagerName,

Member

snicoll Feb 26, 2018

I wonder if we need to bring the complexity of BeansEndpoint here. If you are working with an application, I don't see the use case of having two cache managers with the same name in different context.

As a user, having to specify the context is very annoying isn't it?

...spring-boot-actuator/src/main/java/org/springframework/boot/actuate/cache/CacheEndpoint.java Outdated

    
              		}

              	}

              	private void clearCaches(ApplicationContext context, String cacheManagerName,

Member

snicoll Feb 26, 2018

Naturally I would go with clearing a cache by name and just throw an exception if there are more than one CacheManager defining a cache with that name. Again, I don't think we should bring that complexity to the user for the very few that chose a complicated setup. The name of the cache manager could be used as a selector perhaps?

...spring-boot-actuator/src/main/java/org/springframework/boot/actuate/cache/CacheEndpoint.java Outdated

    
              	 * Description of an application's {@link CacheManager} beans, primarily intended for

              	 * serialization to JSON.

              	 */

              	public static final class ApplicationCacheManagerBeans {

Member

snicoll Feb 26, 2018

I have a feeling those names comes from the BeansEndpoint. I don't think this applies here.

Contributor Author

joshiste commented Feb 26, 2018

@snicoll Thanks for your comments. I've copied some stuff from the liquibaseEndpoint hence the handling of multiple contexts.

Contributor Author

joshiste commented Feb 26, 2018 •

edited

Loading

I've renamed the endpoint to CachesEndpoint.
I've removed the handling of complex context structures.
I've removed the superfluous config metadata.

I didn't make the cacheManager a @Selector since having it as queryParam allows to delete all caches over all cacheManagers.

joshiste force-pushed the caches-endpoint branch from aa4f990 to c193834 Compare

February 26, 2018 08:52

wilkinsona changed the title ~~Add cache actuator endpoint for listing an clearing caches~~ Add cache actuator endpoint for listing and clearing caches

philwebb removed the priority: normal label

snicoll mentioned this pull request

Create a cache actuator endpoint. #2625

Closed

ptahchiev reviewed

View reviewed changes

...pring-boot-actuator/src/main/java/org/springframework/boot/actuate/cache/CachesEndpoint.java Outdated

    
              			cacheManager.getCacheNames().forEach(cn -> cacheManager.getCache(cn).clear());

              		}

              		else {

              			cacheManager.getCache(cacheName).clear();

Contributor

ptahchiev Mar 28, 2018

I believe a null-pointer check is necessary here.

Contributor

ptahchiev commented Mar 28, 2018

Maybe it's a good idea to add also a method for removing a single entry from the Cache region? Would be nice to also list all the entries in a given cache region, but the current spring cache API does not allow it (jcache does it allow it).

philwebb modified the milestones: Icebox, Backlog

Member

snicoll commented Apr 9, 2018

@ptahchiev let's not expand the scope of this PR please. Listing the content of the cache is definitely not at the agenda. We can refine the support once that PR is merged (or you can submit one once this one is merged).

joshiste force-pushed the caches-endpoint branch from c193834 to cb9eb6c Compare

April 10, 2018 05:14

Contributor Author

joshiste commented Apr 10, 2018

Added null check and rebased to master

snicoll added for: team-attention and removed for: team-attention labels

Member

snicoll commented Apr 18, 2018 •

edited

Loading

We've discussed a few things and we'd like a different output format. We'd like to list the caches with a name, potentially adding a reference to the cache manager when they are two caches with the same name. I don't like the idea of adding the penalty of the CacheManager reference where the vast majority of apps only have one and I don't like the idea of using a bean name there either.

We've also discussed that the POST operation should just drop the cache with given names, with the ability to further qualify in case of conflicts (but, IMO, that's a corner case that shouldn't happen).

I am not sure yet how to structure the read operation so we need to give it a bit more thought. We will use this PR as the base for this work.

philwebb added the for: merge-with-amendments label

joshiste force-pushed the caches-endpoint branch from cb9eb6c to 59b3eae Compare

April 18, 2018 19:27

Contributor Author

joshiste commented Apr 18, 2018 •

edited

Loading

I don't like the idea of using a bean name there either.

If we need to qualify the caches in case of duplicates names in two CacheManagers, I guess there is nothing else left to distinct the caches

How about rendering links for cache eviction into the response and just prefixing the cache name in case of ambiguity?
The downside is: it would need a mvc extension for the endpoint and doesn't clarify the usage in case of using the MXBean

{
  "caches": [
    {
      "name": "manager-foo/ambiguousName",
      "_links": {
        "evict": "/actuator/caches/manager-foo/ambiguousName"
      }
    },
    {
      "name": "manager-bar/ambiguousName",
      "_links": {
        "evict": "/actuator/caches/manager-bar/ambiguousName"
      }
    },
    {
      "name": "uniqueName",
      "_links": {
        "evict": "/actuator/caches/manager-bar/uniqueName"
      }
    }
  ]
}

... if the cache name contains a / it need's to be encoded in the link...

Member

snicoll commented Apr 25, 2018 •

edited

Loading

@joshiste what I had in mind was more an attribute that would qualify the cache

{
  "caches": [
    {
      "name": "ambiguousName",
      "cacheManager": "manager-foo"
    },
    {
      "name": "ambiguousName",
      "cacheManager": "manager-bar"
    },
    {
      "name": "uniqueName",
      "cacheManager": "manager-foo"
    }
  ]
}

Those links are also not very welcome there (if we want to do this we should do this consistently and at the endpoint API level as those links are useless and irrelevant for JMX).

joshiste added 4 commits

April 25, 2018 16:13


          Add cache actuator endpoint for listing an clearing caches

8916ae5

This commits adds an actuator endpoint which lists the caches per
context and cacheManager and provides a delete operation to clear the
caches. As the statistics are exposed via the metrics endpoint they are
not included

closes spring-projects#2625


          Do the changes as suggested by snicoll

2bcf4a2

- rename to CachesEndpoint
- remove handling of complex Contexts
- remove unecessary config metadata


          Add null check

92fe904


          Change the endpoint output as suggested by snicoll

253a65b

joshiste force-pushed the caches-endpoint branch from 59b3eae to 253a65b Compare

April 25, 2018 14:49

Contributor Author

joshiste commented Apr 25, 2018

I've modified the output of the read operation as you suggested.

snicoll self-assigned this

snicoll removed the for: merge-with-amendments label

snicoll modified the milestones: Backlog, 2.1.0.M1

snicoll pushed a commit that referenced this pull request


          Add cache actuator endpoint

1a57673

This commits adds an actuator endpoint which lists the caches per
context and cacheManager and provides a delete operation to clear the
caches. As the statistics are exposed via the metrics endpoint they are
not included

See gh-12216

snicoll closed this in

fb8a5a9

snicoll added a commit that referenced this pull request


          Merge pull request #12216 from Mon

bb614b5

* pr/12216:
  Polish "Add cache actuator endpoint"
  Add cache actuator endpoint

Member

snicoll commented Apr 30, 2018

@joshiste thanks for the PR. While reviewing, I've reworked and significantly changed the initial proposal, see fb8a5a9 for more details.

Contributor Author

joshiste commented Apr 30, 2018

Yay! Thanks for merging!

snicoll mentioned this pull request

Improve caches endpoint format #13079

Closed

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

Labels

type: enhancement