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 note about EntryProcessor thread safety in Javadoc #6593

Closed
wimdeblauwe opened this issue Oct 28, 2015 · 10 comments

Comments

Projects
None yet
5 participants
@wimdeblauwe
Copy link

commented Oct 28, 2015

This note:

NOTE: An Entry Processor instance is not thread safe. If you are storing partition specific state between invocations, be sure to register this in a thread-local. An Entry Processor instance can be used by multiple partition threads.

should be added to the javadoc of Entry Processor. It is currently a bit hidden in the documentation. It would be better to also have it in the javadoc for people that look there first.

This is especially important for Hazelcast users that need to work around the issue #6578

@fschuh

This comment has been minimized.

Copy link

commented Oct 28, 2015

I also feel this is not very clear and should be better explained in the docs.
If I invoke single-shot EntryProcessor (i.e. its instance is called only once and then destroyed), how could it be used by multiple partition threads?
I can see it being used by another node (in the case of the Backup processor), but how could it be reused by another partition thread in the same node?

@wimdeblauwe

This comment has been minimized.

Copy link
Author

commented Oct 28, 2015

If you call map.executeOnKeys( keySet, new MyEntryProcessor() ) then I have noticed that this same instance of MyEntryProcessor will be hit by multiple threads (using a different key in different threads)

@pveentjer

This comment has been minimized.

Copy link
Member

commented Oct 28, 2015

+1

I'll add it to the documentation.

@pveentjer

This comment has been minimized.

Copy link
Member

commented Oct 28, 2015

For the time being adding a fix to the documentation is the simplest, but I think in the near future we should just give every partition its own EntryProcessor instance and then we don't need to deal with threadlocals at all.

@gurbuzali gurbuzali added this to the 3.6 milestone Oct 28, 2015

@fschuh

This comment has been minimized.

Copy link

commented Oct 29, 2015

Good point on map.executeOnKeys().
Agreed that we shouldn't have to deal with threadlocals, but giving each partition its own EntryProcessor instance might not always be desirable, especially when there's no local state stored by the EntryProcessor (and its backup) at all.
As @wimdeblauwe mentioned though, issue #6578 as of now requires at least a bit of local state manipulation, so it's very hard to avoid using a threadlocal unless Hazelcast solves #6578 altogether.

Also, for giving each partition its own instance, if the EntryProcessor contains complex parameters inside then some deep cloning or other instantiation method would be necessary.
It would be nice to have per-partition instances as a configurable option though.

@pveentjer

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

I'm still thinking about it. The question is.. when is it relevant to share state in a partition, but when not between partitions.

@wimdeblauwe could you give some insights in the actual problem you are trying to solve and you are now relying on threadlocals?

@wimdeblauwe

This comment has been minimized.

Copy link
Author

commented Oct 29, 2015

@pveentjer sure. There are 2 things I need to solve:

  1. I want to know if the entry processor actually updated a value or not. If the entry processor did not update anyting in the cache, I want to return null for the backup processor since there is no use that it runs.

  2. I want to know if the entry processor received a non-null value. If this was the case and the backup entry processor receives a null value, then we have to skip the processing in the backup processor (Due to #6578 )

If both issues would be handled by the framework (For issue 1, that would mean Hazelcast would not call into the backup processor if there was no update anyway. For issue 2, I don't know), then no ThreadLocals would be needed (or cloning of EntryProcessors).

@pveentjer

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

I understand; it makes sense. I didn't think about the backup method.

@mdogan mdogan modified the milestones: 3.7, 3.6 Nov 27, 2015

@mdogan mdogan added the Team: Core label Nov 27, 2015

@pveentjer

This comment has been minimized.

Copy link
Member

commented May 8, 2016

I'm going to close this ticket since it has been addressed in the javadoc of the EntryProcessor. @wimdeblauwe can you open a new ticket for a more advanced solution.

@pveentjer pveentjer closed this May 8, 2016

@wimdeblauwe

This comment has been minimized.

Copy link
Author

commented May 9, 2016

@pveentjer I am no longer using Hazelcast due to changing of jobs. I suggest to open the ticket yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.