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.

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

ISPN-9155 clear() should not send remove notifications #6036

Closed
wants to merge 1 commit into from
Closed

ISPN-9155 clear() should not send remove notifications #6036

wants to merge 1 commit into from

Conversation

galderz
Copy link
Member

@galderz galderz commented Jun 4, 2018

https://issues.jboss.org/browse/ISPN-9155

Wait for CI to see what I've broken ;)

@galderz galderz requested a review from wburns June 4, 2018 16:15
@tristantarrant
Copy link
Member

Do we want to add the complementary removeAll() that does keep these semantics ?

@wburns
Copy link
Member

wburns commented Jun 4, 2018

We should mention this in the upgrading guide as well. https://github.com/infinispan/infinispan/blob/master/documentation/src/main/asciidoc/upgrading/upgrading.asciidoc

danberindei
danberindei reviewed Jun 4, 2018
View reviewed changes
documentation/src/main/asciidoc/user_guide/cache_api.adoc
==== Clearing the cache

The fastest way to clear a cache is to call
link:{javadocroot}/org/infinispan/Cache.html#clear--[clear] method.
Copy link
Member

Choose a reason for hiding this comment

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

Should we recommend clear() here? The javadoc still says "This should never be invoked in production"

documentation/src/main/asciidoc/user_guide/cache_api.adoc
IMPORTANT: Starting with version 9.3, calling `clear` does no longer emit remove notifications for each removed key.
If relying on removed events being fired upon calling `clear`, it is recommended to call `cache.keySet().stream().forEach(Cache::remove)`.
Removing all entries in this way is optimized so that with distributed caches, removes happen in parallel.
Further optimization can be achieved by calling `disableRehashAware` on the stream.
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't say "optimized" here, since it's always going to be much slower than the old clear()

core/src/main/java/org/infinispan/Cache.java
*
* Starting with version 9.3, calling this method does not result if cache entry removed events being fired.
* If you rely on cache entry removed events
* , use {@link CacheStream#forEach(BiConsumer)} to individually remove each entry and get remove notifications.
Copy link
Member

Choose a reason for hiding this comment

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

We should document this in the CacheEntryRemoved javadoc as well, and check that we don't implicitly rely on the listeners being invoked internally (e.g. MigrationTask, global state, L2, JCache/Spring adapters)

@danberindei
Copy link
Member

@galderz as I mentioned in JIRA, we already have SKIP_LISTENER_NOTIFICATION, so I don't see a really good reason for this.

At the very least we should make sure we never use clear+listeners internally, and add extra warnings where we add listeners internally and the user can call clear() directly without necessarily knowing about our internal listeners.

@wburns
Copy link
Member

wburns commented Jun 4, 2018

My guess is that the issue is that RemoteCache has no way to ignore listeners on a clear invocation. Since clear is a lot slower when any cluster listener is installed (including remote listener).

@danberindei
Copy link
Member

Speaking of RemoteCache, how would the near cache deal with cache.clear() from an embedded node or from another client?

@anistor
Copy link
Member

anistor commented Jun 4, 2018

My vote for this: -1. I've added my concerns in the jira comments.

@gustavocoding
Copy link

Just an idea: why not keep the current method "as is" to avoid breaking clients, and provide another one named truncate, purge or whatever, documenting on the javadoc that it is a "fast" clean?

@tristantarrant
Copy link
Member

truncate() would be my choice if we keep clear() as is

@galderz galderz force-pushed the t_9155 branch 2 times, most recently from f6ea1e9 to d3f12c5 Compare June 12, 2018 06:56
@danberindei
Copy link
Member

truncate() is better because it doesn't affect existing callers, but I'm still not convinced that we need a new method that does the same thing as clear()+SKIP_LISTENER_NOTIFICATION. Remember, the clear() javadoc still recommends not using it in production:

 * <b>Note:</b> This should never be invoked in production unless you can guarantee no other invocations are ran
 * concurrently.

@gustavocoding
Copy link

@danberindei usability/syntactic sugar? truncate() would be implemented internally as clear()+SKIP_LISTENER_NOTIFICATION

@galderz
Copy link
Member Author

galderz commented Jun 13, 2018

I'm not totally sure I understand why you all are so worried about changing the semantics of clear. We already changed the semantics before (in a minor release too) (ISPN-5370) and I don't recall such a palava (here and here).

Sure, this PR has more impact on the users since it'd affect remove notifications, but it's clear() we're talking about. It's already defined pretty liberally, even more so after ISPN-5370.

@anistor
Copy link
Member

anistor commented Jun 13, 2018

It's about that more impact that we're concerned about. It does not matter if we're doing this in a minor or major, it's bad anyway because it breaks stuff. Comparing this to a precedent like ISPN-5370 is not ideal because ISPN-5370 was a compromise needed in order to fix really badly broken things.

I think implementing the proposed clear()+SKIP_LISTENER_NOTIFICATION is more than enough good compromise and should make everybody happy. Btw, we should ensure the skip_listener_notification flag works with any write command.

@danberindei
Copy link
Member

@galderz I'm not that worried about the user listeners, at some point they'll put a breakpoint in their listeners and see they're not invoked during clear. I'm more worried that maybe we use listeners internally for some other features (near cache?), and the connection between those features and clear() is not obvious to the users. Maybe it's less of a reason to keep clear() as is and more of a reason to not use listeners internally :)

@galderz
Copy link
Member Author

galderz commented Jun 15, 2018

I think implementing the proposed clear()+SKIP_LISTENER_NOTIFICATION is more than enough good compromise and should make everybody happy. Btw, we should ensure the skip_listener_notification flag works with any write command.

Flag.SKIP_LISTENER_NOTIFICATION does not exist for remote caches.

@galderz
Copy link
Member Author

galderz commented Jun 15, 2018

Nor makes sense to add such flag.

@tristantarrant tristantarrant added this to the 9.4.0.Alpha1 milestone Jun 20, 2018
@tristantarrant
Copy link
Member

I'm postponing this for 9.4 as I'd like to get full agreement from everybody on the solution

@galderz galderz force-pushed the t_9155 branch 3 times, most recently from 2dfd2c1 to 41365cb Compare June 25, 2018 09:21
@galderz galderz closed this Jul 3, 2018
@galderz galderz deleted the t_9155 branch July 12, 2018 11:14
@wburns wburns mentioned this pull request Oct 12, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danberindei danberindei danberindei left review comments

@wburns wburns Awaiting requested review from wburns

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
9.4.0.Alpha1
6 participants
@galderz @tristantarrant @wburns @danberindei @anistor @gustavocoding