[1.2.0] Event ordering [API-1102] #696

utku-caglayan · 2021-12-05T14:24:52Z

Fixes #692

Hazelcast Cluster members, respects the order of the events. Different order guarantees are given for different event types (see docs for detail)

Java Client summarizes these guarantees as "Order of the events originated from the same partition is preserved". For example events which belong to the same key of a map are received(on the clients) in the same order as they created. This is achieved via StripeExecutor implementation which is also mentioned in the link above.

Java Client processes incoming events on a StripedExecutor, by treating event handlers as "stripedRunnable"s with respect to their partitionID. Please check it for reference.

This PR fixes event processing (execution of event-handlers) logic by respecting the order of events from the same partition.

Preserve order of execution of events handler which originates from the same partition

codecov-commenter · 2021-12-06T07:45:55Z

Codecov Report

Merging #696 (76ee128) into master (40b05ce) will decrease coverage by 2.20%.
The diff coverage is 87.71%.

@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
- Coverage   74.37%   72.17%   -2.21%     
==========================================
  Files         311      330      +19     
  Lines       13571    15214    +1643     
==========================================
+ Hits        10094    10981     +887     
- Misses       2739     3458     +719     
- Partials      738      775      +37

Impacted Files	Coverage Δ
internal/invocation/invocation_service.go	`80.64% <76.92%> (+2.02%)`	⬆️
internal/invocation/stripe_executor.go	`90.90% <90.90%> (ø)`
internal/cluster/address_provider.go	`66.66% <0.00%> (-20.00%)`	⬇️
internal/proto/codec/builtin.go	`45.16% <0.00%> (-19.46%)`	⬇️
internal/cloud/address_provider.go	`28.57% <0.00%> (-14.29%)`	⬇️
internal/cluster/cluster_service.go	`72.80% <0.00%> (-5.56%)`	⬇️
internal/serialization/default_portable_reader.go	`92.18% <0.00%> (-5.33%)`	⬇️
client.go	`56.95% <0.00%> (-3.32%)`	⬇️
internal/serialization/object_data.go	`88.14% <0.00%> (-2.60%)`	⬇️
internal/event/dispatch_service.go	`80.00% <0.00%> (-2.50%)`	⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40b05ce...76ee128. Read the comment docs.

internal/invocation/invocation_service.go

emreyigit · 2021-12-17T13:54:33Z

I think we should add more details to PRs. We experienced similar situation recently at .net side. @ihsandemir suggested it. Otherwise, reviewing becomes frustrating.

yuce

Here's some initial comments, will continue to review.

cluster/event_config.go

internal/invocation/stripe_executor.go

internal/invocation/stripe_executor_test.go

internal/invocation/stripe_executor.go

internal/invocation/stripe_executor_test.go

client_it_test.go

internal/invocation/invocation_service.go

yuce · 2021-12-29T16:07:03Z

internal/invocation/stripe_executor.go

+}
+
+// dispatch sends the handler "task" to one of the appropriate taskQueues, "tasks" with the same key end up on the same queue. Returns true if queue is full and could not dispatch
+func (se stripeExecutor) dispatch(key int, task func()) (queueFull bool) {


I think this function should return false when the task cannot be dispatched. That could also help getting rid of the return variable name.

It is definitely more idiomatic thanks, addressed it at 3bb2d0f

client_it_test.go

yuce · 2021-12-30T11:07:10Z

client_it_test.go

@@ -140,6 +143,64 @@ func TestClientMemberEvents(t *testing.T) {
 	})
 }

+func calcPartitionID(ss *serialization.Service, key interface{}) (int32, error) {


I think this function may be named better. Maybe calculatePartitionID as the last resort.

Sadly, could not come up with a better one. Renamed at 5fadb18

yuce · 2021-12-30T11:24:06Z

internal/invocation/stripe_executor.go

+}
+
+// newStripeExecutor returns a new stripeExecutor with configured queueCount and queueSize.
+func newStripeExecutorWithConfig(queueCount, queueSize int) (stripeExecutor, error) {


How about returning only the stripeExecutor? error is always nil. We can update it if in the future we need to return an error. Also, should it return *stripeExecutor?

Got rid off error at 67d42e5

Refactored stripeExecutor to pointer semantics as you suggested at f8f778f

internal/invocation/stripe_executor_test.go

yuce · 2021-12-30T11:31:14Z

internal/invocation/stripe_executor_test.go

+		queueCount    int
+		key           int
+		expectedIndex int
+	}{


Should there be a test case for key == -1 as well? If it's hard to do that in this function due to random stuff, at the very least we should have a separate test that checks dispatch works with negative integers too.

Great idea! Created a separate test for that at 99b7971

yuce · 2021-12-30T11:36:41Z

internal/invocation/stripe_executor_test.go

+	go func() {
+		for _, task := range tasks {
+			if ok := se.dispatch(task.key, task.handler); !ok {
+				panic("could not dispatch event handler")
+			}
+		}
+	}()


Can you refactor this, so each task is dispatched by a different goroutine?

List of tasks is created in a way that randomizes tasks but respect the order of them with respect to their keys, just like the situation on clusters. If we dispatch each one in a different goroutine, we cannot guarantee the order among the tasks of the same key.

client_it_test.go

yuce · 2021-12-30T11:38:04Z

client_it_test.go

+	for i := 1; i <= 1000; i++ {
+		it.MustValue(m.Put(ctx, i, "test"))
+	}


Could you refactor this so m.Puts run concurrently?

Instead implemented a blackbox one as we discussed df8e738

Use partition id of event ClientMessage, instead of stored "AddListener" invocation partition id which is 0 since it is not addressed to a specific partition

…dering logic

yuce

Thanks, looks good!

utku-caglayan added 6 commits December 5, 2021 15:35

remove unused field

f40f3fe

fix typo

134b168

Initial impl stripe executor impl:

e604b7e

Preserve order of execution of events handler which originates from the same partition

handle executor start and stop

33e3d78

handle partitionID -1

ac232b7

fix field alignment issues

2563ce8

utku-caglayan requested review from yuce and emreyigit December 14, 2021 06:01

emreyigit reviewed Dec 17, 2021

View reviewed changes

internal/invocation/invocation_service.go Outdated Show resolved Hide resolved

emreyigit reviewed Dec 17, 2021

View reviewed changes

internal/invocation/invocation_service.go Outdated Show resolved Hide resolved

introduce "noSpecificPartition" variable to improve readability

b546c1d

utku-caglayan requested a review from emreyigit December 21, 2021 06:58

utku-caglayan added 5 commits December 21, 2021 12:47

make queue size and worker count configurable

c4dc047

Merge branch 'master' into event-ordering

946ba9c

temporary fix for memory leak test

eff8e8f

decrease queue capacity, and document the differences

22155c9

decrease queue capacity, and document the differences

2f91eed

yuce changed the title ~~Event ordering~~ [1.2.0] Event ordering Dec 23, 2021

yuce requested changes Dec 24, 2021

View reviewed changes

utku-caglayan added 5 commits December 24, 2021 13:26

remove event executor configuration

16dd0b2

address pr reviews for stripe executor

7ae872c

fix failing "TestMarshalDefaultConfig" test

158c62e

fix failing test

3491021

rename "ind" to "i"

c1efb0c

utku-caglayan force-pushed the event-ordering branch from c3dd483 to c1efb0c Compare December 27, 2021 01:06

yuce reviewed Dec 28, 2021

View reviewed changes

client_it_test.go Outdated Show resolved Hide resolved

yuce reviewed Dec 28, 2021

View reviewed changes

internal/invocation/invocation_service.go Outdated Show resolved Hide resolved

yuce reviewed Dec 28, 2021

View reviewed changes

internal/invocation/invocation_service.go Show resolved Hide resolved

yuce changed the title ~~[1.2.0] Event ordering~~ [1.3.0] Event ordering Dec 29, 2021

yuce removed this from the 1.2.0 milestone Dec 29, 2021

utku-caglayan added 4 commits December 29, 2021 18:09

refactor int32 to int

f7df88d

refactor TestClientEventHandlingOrder logic

03fe449

change behavior to not block on "dispatch" if event queue is full

1c7a575

remove unused func

2b65d5a

yuce reviewed Dec 29, 2021

View reviewed changes

yuce added this to the 1.2.0 milestone Dec 30, 2021

refactor return of "dispatch"

3bb2d0f

utku-caglayan force-pushed the event-ordering branch from e38029e to 3bb2d0f Compare December 30, 2021 09:35

undo older bad merge

ab26f81

emreyigit approved these changes Dec 30, 2021

View reviewed changes

yuce reviewed Dec 30, 2021

View reviewed changes

client_it_test.go Outdated Show resolved Hide resolved

yuce reviewed Dec 30, 2021

View reviewed changes

yuce requested changes Dec 30, 2021

View reviewed changes

utku-caglayan added 10 commits December 30, 2021 16:28

fix test to start partitionIDs from 0

6faa426

improve tests

db4dcd6

add dispatch zero&negative key test

99b7971

refactor constructor to raise panic on invalid conf

67d42e5

refactor test for stability

4d0c38c

refactor stripeExecutor to pointer semantics

f8f778f

minor improvement

50b5bd5

fix major bug

feab9ed

Use partition id of event ClientMessage, instead of stored "AddListener" invocation partition id which is 0 since it is not addressed to a specific partition

implement a blackbox event order test that fails on previous event or…

df8e738

…dering logic

rename a helper func

5fadb18

yuce changed the title ~~[1.3.0] Event ordering~~ [1.2.0] Event ordering Dec 31, 2021

refactor struct for padding

76ee128

yuce approved these changes Dec 31, 2021

View reviewed changes

utku-caglayan merged commit 649a34c into hazelcast:master Dec 31, 2021

utku-caglayan changed the title ~~[1.2.0] Event ordering~~ [1.2.0] Event ordering [API-1102] Dec 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.2.0] Event ordering [API-1102] #696

[1.2.0] Event ordering [API-1102] #696

utku-caglayan commented Dec 5, 2021 •

edited

Loading

codecov-commenter commented Dec 6, 2021 •

edited

Loading

emreyigit commented Dec 17, 2021

yuce left a comment

yuce Dec 29, 2021

utku-caglayan Dec 30, 2021

yuce Dec 30, 2021

utku-caglayan Dec 30, 2021

yuce Dec 30, 2021

utku-caglayan Dec 30, 2021

utku-caglayan Dec 30, 2021

yuce Dec 30, 2021

utku-caglayan Dec 30, 2021

yuce Dec 30, 2021

utku-caglayan Dec 30, 2021

yuce Dec 30, 2021

utku-caglayan Dec 30, 2021

yuce left a comment

[1.2.0] Event ordering [API-1102] #696

[1.2.0] Event ordering [API-1102] #696

Conversation

utku-caglayan commented Dec 5, 2021 • edited Loading

codecov-commenter commented Dec 6, 2021 • edited Loading

Codecov Report

emreyigit commented Dec 17, 2021

yuce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuce left a comment

Choose a reason for hiding this comment

utku-caglayan commented Dec 5, 2021 •

edited

Loading

codecov-commenter commented Dec 6, 2021 •

edited

Loading