-
Notifications
You must be signed in to change notification settings - Fork 35
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
docs: First iteration of active-active guide steps #876
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good
akka-distributed-cluster-docs/src/main/paradox/guide/3-active-active.md
Outdated
Show resolved
Hide resolved
akka-distributed-cluster-docs/src/main/paradox/guide/3-active-active.md
Outdated
Show resolved
Hide resolved
akka-distributed-cluster-docs/src/main/paradox/guide/3-active-active.md
Outdated
Show resolved
Hide resolved
akka-distributed-cluster-docs/src/main/paradox/guide/3-active-active.md
Outdated
Show resolved
Hide resolved
akka-distributed-cluster-docs/src/main/paradox/guide/3-active-active.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good
Because of this the data structure must always arrive at the same state even if events arrive in a different order. To handle | ||
this in the shopping cart we replace the `ItemAdded` and `ItemRemoved` events with a single `ItemUpdated` event | ||
Because of this the data structure must always arrive at the same state even if events arrive in a different order. We have | ||
already prepared for this somewhat by representing both add and remove using a single `ItemUpdated` event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a single event is not really a requirement for RES so I'm not sure about this explanation. The previous problem was the validation that Add could only be once, and then Adjust.
Another thing, do we need to clarify that events from a certain replica are always ordered? It's not that events are completely randomly ordered, there are some happens-before rules. Would maybe be too much detail here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was more my thought process of aligning the samples that leaked into the text than something actually requird, it was either pulling add/remove in here or the other way around. I'll see about making that more concise/correct.
I think talking about same-order from same replica would be to low level here, we already cover that in the Akka docs https://doc.akka.io/docs/akka/current/typed/replicated-eventsourcing.html#causal-delivery-order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully better now b7a4f5a
return shopping.cart.proto.ItemRemoved.newBuilder() | ||
.setCartId(itemRemoved.cartId) | ||
.setItemId(itemRemoved.itemId) | ||
.setCartId(PersistenceId.extractEntityId(envelope.persistenceId())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this? We still have the cartId in the event, and I think that is a pretty good practise to include the id in the event even though it technically can be extracted from the envelope like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, it was removed from the "internal" events but still in the external protobuf event.
That seems good, and also illustrates something that the transformation can handle different representations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that the cart id was only included in the events to be able to pass it in the events published by the gRPC projection but that is now possible with the envelope transform without having to tweak the ES events just for the projection capability.
items.put(itemId, quantity); | ||
} | ||
int newQuantity = items.getOrDefault(itemId, 0) + quantity; | ||
items.put(itemId, newQuantity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason why we put 0 items, and then filter in other places? could it remove here instead if 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was mostly to keep it roughly the same with the RES version, we could remove instead here since it is a more natural design for non RES.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a reason why it can't remove also in RES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If add 1, remove 1, add 1, remove 1 wold reordered to add 1, remove 1, remove 1, add 1 we get different end results with RES?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back to removing non-present in grpc projection samples in db20600
@patriknw I added opt in vip-replication to the Scala RES sample, but it requires an extra |
I think that is fine. We are using the same sample to illustrate two different things and that's why it's not as clean as we may want, but don't see a good alternative right now. |
1a1b66b
to
3ab5cb4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have read the full guide steps and I think it's looking good. Some minor comments...
akka-distributed-cluster-docs/src/main/paradox/guide/1-event-sourced-shopping-cart.md
Outdated
Show resolved
Hide resolved
akka-distributed-cluster-docs/src/main/paradox/guide/2-service-to-service.md
Outdated
Show resolved
Hide resolved
akka-distributed-cluster-docs/src/main/paradox/guide/2-service-to-service.md
Outdated
Show resolved
Hide resolved
@@ -34,15 +34,26 @@ Java | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The State has customerId
and customerCategory
. First I thought that was for the RES filter, but I think that is vipCustomer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover from something that was before, I missed out on dropping it on the Scala side. We sort of re-introduce category in the RES sample for filtering but I made that a boolean for simplicity. Fixed in 4dd9c03
curl http://localhost:9201/ready | ||
``` | ||
|
||
6. Try it with [grpcurl](https://github.com/fullstorydev/grpcurl): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something wrong with the rendering from step 6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, missing block end, fixed 7de94a4
Co-authored-by: Patrik Nordwall <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I only reviewed the Markdown files though.
I left some suggestions. Mostly commas.
akka-distributed-cluster-docs/src/main/paradox/guide/1-event-sourced-shopping-cart.md
Outdated
Show resolved
Hide resolved
akka-distributed-cluster-docs/src/main/paradox/guide/3-active-active.md
Outdated
Show resolved
Hide resolved
akka-distributed-cluster-docs/src/main/paradox/guide/3-active-active.md
Outdated
Show resolved
Hide resolved
akka-distributed-cluster-docs/src/main/paradox/guide/3-active-active.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Renato Cavalcanti <[email protected]>
test fail was #834 (comment), which seems to be flaky since before |
No description provided.