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

Entry listener with predicate should be notified on value entry/exit from predicate value space #8340

Merged
merged 3 commits into from Jul 7, 2016

Conversation

Projects
None yet
4 participants
@vbekiaris
Copy link
Contributor

commented Jun 7, 2016

This PR introduces filtering strategies in map event publisher, to facilitate building a 'query cache' style listener with predicate on IMap . Default filtering strategy maintains Hazelcast entry events behavior. A new filtering strategy has been introduced which processes UPDATED entry events and, depending on listener's predicate, may alter the type of event, to match exit/entry of value from predicate value space.

An EE counterpart is required.

@vbekiaris vbekiaris added the Team: Core label Jun 7, 2016

@vbekiaris vbekiaris added this to the 3.7 milestone Jun 7, 2016

@vbekiaris vbekiaris self-assigned this Jun 7, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2016

Test PASSed.

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch from 04430c5 to 700a17c Jun 8, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2016

Test FAILed.

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch from 700a17c to 188c68d Jun 8, 2016

@vbekiaris vbekiaris changed the title [DONT MERGE] Entry listener with predicate should be notified on value entry/exit from predicate value space Entry listener with predicate should be notified on value entry/exit from predicate value space Jun 8, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2016

Test FAILed.

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2016

IssuesTest::testIssue321_3 fails due to expectation that an entry event which include values will be published before the same event is published to another listener who excludes values. Since there are no event delivery order guarantees, changing the test to verify one event with value and another event without value are received by the listener, in any order.

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch from 188c68d to 605076a Jun 8, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2016

Test PASSed.

Object mergingValue) {

Map<Integer, EntryEventData> eventDataIncludingValues = new Int2ObjectHashMap<EntryEventData>(EVENT_DATA_MAP_CAPACITY);
Map<Integer, EntryEventData> eventDataExcludingValues = new Int2ObjectHashMap<EntryEventData>(EVENT_DATA_MAP_CAPACITY);

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Jun 10, 2016

Member

We can defer Int2ObjectHashMap creation at a later point, and can do it when needed, now for ex., if i have value-included event registrations and don't have any value-excluded ones, there will be always two Int2ObjectHashMap instance creation although one is sufficient. This event publishing part is on hot-path and it is better to create possible minimum number of objects.

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Jun 14, 2016

Author Contributor

Good point, deferred map creation until actually needed.

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch from 605076a to 5cae199 Jun 14, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

Test FAILed.

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch from 5cae199 to ddd6f6a Jun 14, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

Test PASSed.

Object mergingValue) {

Map<Integer, EntryEventData> eventDataIncludingValues = null;
Map<Integer, EntryEventData> eventDataExcludingValues = null;

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Jun 21, 2016

Member

isn't an ArrayList cheaper than Map to hold event data in terms of number of created objects and calculations needed for hash? Seems an ArrayList is a better fit here and no need a Map

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Jun 21, 2016

Author Contributor

The need here is to maintain a mapping from event type of event-to-be-published to the EntryEventData object (which can be reused across events published to listeners). At the moment, given the filtering strategies implemented in this PR, in the worst case for an UPDATED event we might create 2 more entries for event types REMOVED and ADDED (when the QueryCacheNaturalFilteringStrategy is used and appropriately configured listeners with predicates are registered).
On the other hand, with the default filtering strategy, there is no need to use a map at all, we just need to maintain two possible forms of the EntryEventData (with & without values), so in this case the Maps are indeed overkill.
Caching EntryEventData is not a direct concern of the chosen filtering strategy, however the caching implementation can be optimized depending on the chosen filtering strategy, so I will try something like introducing a new interface EntryEventDataCache, with an instance obtainable from the FilteringStrategy itself:

interface EntryEventDataCache {
  EntryEventData getOrCreateEventData(mapName, caller, dataKey, value, oldValue, mergingValue, eventTypeForPublishing);
}

interface FilteringStrategy {
....
   EntryEventDataCache getEntryEventDataCache();
}
int orderKey = pickOrderKey(dataKey);
publishEventInternal(includeValueRegistrations, eventData, orderKey);
// if events were generated, execute the post-publish hook on each one
if (eventDataIncludingValues != null) {

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Jun 21, 2016

Member

what about eventDataExcludingValues? is it also needed postPublishEvent phase?

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Jun 21, 2016

Author Contributor

With current subclass implementation, no it's not needed, however in order to future-proof the implementation (and taking into account the following comment), we could amend postPublishEvent as postPublishEvent(Collection<EntryEventData> eventDataIncludingValues, Collection<EntryEventData> eventDataExcludingValues).

if (eventDataIncludingValues != null) {
for (EntryEventData entryEventData : eventDataIncludingValues.values()) {
postPublishEvent(entryEventData);
}

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Jun 21, 2016

Member

To prevent unnecessary iterations, signature can be changed to receive a collection postPublishEvent(Collection<EntryEventData> eventData)

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

Test FAILed.

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch from e7e2188 to d58b51b Jun 22, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

Test FAILed.

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch 2 times, most recently from d58c92b to 49c8653 Jun 22, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

Test PASSed.

1 similar comment
@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

Test PASSed.

@ahmetmircik

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

Maybe you want to remove some unused methods in MapEventPublisherImpl, stumbled them when i get your branch my local,

other than that LGTM 👍

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch from 49c8653 to ae93547 Jun 22, 2016

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2016

Good catch, thanks for pointing that out.

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

Test FAILed.

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2016

The MapEventPublisherImplTest committed previously erroneously tests that EntryEvent objects received by listeners are the same, however this is not the case: even though a single EntryEventData object is used to dispatch the event to listeners, ultimately a new DataAwareEntryEvent object is delivered to each listener. Working on replacing this test with another one that actually tests the operation of each EntryEventDataCache implementation.

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch from ae93547 to 42809d9 Jun 23, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2016

Test PASSed.

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2016

run-lab-run

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

Test PASSed.

@Override
public int doFilter(EventFilter filter, Data dataKey, Object dataOldValue, Object dataValue, EntryEventType eventType,
String mapNameOrNull) {
if (filter instanceof MapPartitionLostEventFilter) {

This comment has been minimized.

Copy link
@tombujok

tombujok Jun 29, 2016

Contributor

Why can the mapName be null?

* <td>Match</td>
* <td>ADDED</td>
* </tr>
* </table>

This comment has been minimized.

Copy link
@tombujok

tombujok Jun 29, 2016

Contributor

Would be great to have such a behaviour-describing table in the DefaultEntryEventFilteringStrategy so that one can compare.

// | Match | Match | UPDATED |
// | Mismatch | Mismatch | NO MATCH|
// | Mismatch | Match | ADDED |
// +-----------+-----------+---------+

This comment has been minimized.

Copy link
@tombujok

tombujok Jun 29, 2016

Contributor

ok, it's here, but maybe it's better to have it in the javadoc too?

@tombujok

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2016

👍 good job (some minor comments)

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2016

Thanks @ahmetmircik & @tombujok, I pushed updated javadoc & fixed the argument name that was leftover as mapNameOrNull. When green, will squash & commit.

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2016

Test PASSed.

private Extractors getExtractorsForMapName(String mapName) {
if (mapName == null) {
return Extractors.empty();
}

This comment has been minimized.

Copy link
@tombujok

tombujok Jun 29, 2016

Contributor

Aha, one more thing. If the mapName can't be null (since you changed it in the last commit) is this null check here necessary?

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Jul 4, 2016

Author Contributor

This null check was introduced in faa6428 but I don't think there are any chances mapName could be null as all code paths arrive at this point via one of publishEvent variants and the callers are various map operations, which should have a non-null map name.

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2016

Test FAILed.

vbekiaris added some commits May 19, 2016

Introduces filtering strategies in map event publisher. Default filte…
…ring strategy maintains Hazelcast entry events behavior. New filtering strategy introduced which processes UPDATED entry events and, depending on listener's predicate, may alter the type event, to match exit/entry of value from predicate value space.
@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2016

Unrelated test failure com.hazelcast.durableexecutor.DurableExecutorServiceTest.testStatsIssue2039:

java.util.concurrent.ExecutionException: java.util.concurrent.RejectedExecutionException: Capacity[1] is reached!!! 
    at com.hazelcast.durableexecutor.impl.TaskRingBuffer.findEmptySpot(TaskRingBuffer.java:70)
    at com.hazelcast.durableexecutor.impl.TaskRingBuffer.add(TaskRingBuffer.java:60)
    at com.hazelcast.durableexecutor.impl.DurableExecutorContainer.execute(DurableExecutorContainer.java:59)
    at com.hazelcast.durableexecutor.impl.operations.TaskOperation.run(TaskOperation.java:47)
    at com.hazelcast.spi.impl.operationservice.impl.OperationRunnerImpl.run(OperationRunnerImpl.java:182)
    at com.hazelcast.spi.impl.operationexecutor.impl.OperationThread.process(OperationThread.java:122)
    at com.hazelcast.spi.impl.operationexecutor.impl.OperationThread.run(OperationThread.java:102)
    at ------ submitted from ------.(Unknown Source)
    at com.hazelcast.spi.impl.operationservice.impl.InvocationFuture.resolve(InvocationFuture.java:111)
    at com.hazelcast.spi.impl.operationservice.impl.InvocationFuture.resolveAndThrow(InvocationFuture.java:74)
    at com.hazelcast.spi.impl.AbstractInvocationFuture.get(AbstractInvocationFuture.java:158)
    at com.hazelcast.durableexecutor.impl.DurableExecutorServiceProxy.submitToPartition(DurableExecutorServiceProxy.java:243)
    at com.hazelcast.durableexecutor.impl.DurableExecutorServiceProxy.submit(DurableExecutorServiceProxy.java:131)
    at com.hazelcast.durableexecutor.DurableExecutorServiceTest.testStatsIssue2039(DurableExecutorServiceTest.java:452)
@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2016

run-lab-run

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2016

Test PASSed.

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch from a9b7d9a to 08f0f91 Jul 6, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2016

Test PASSed.

@vbekiaris vbekiaris force-pushed the vbekiaris:master-event-predicate branch from 08f0f91 to 2fa7d9a Jul 7, 2016

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

Test PASSed.

@vbekiaris vbekiaris merged commit c57d334 into hazelcast:master Jul 7, 2016

1 check passed

default Build finished. 14263 tests run, 179 skipped, 0 failed.
Details

@vbekiaris vbekiaris deleted the vbekiaris:master-event-predicate branch Jul 7, 2016

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.