-
Notifications
You must be signed in to change notification settings - Fork 1
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
Added reactive version of customer service. #1
Conversation
|
||
return customerRepository | ||
.save(customerWithEvents.result) | ||
.flatMap(customer -> domainEventPublisher.publish(Customer.class, customer.getId(), customerWithEvents.events).collectList().map(messages -> customer)) |
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.
.map(messages -> customer)
-> https://projectreactor.io/docs/core/release/api/reactor/core/publisher/Mono.html#thenReturn-V-
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 will fix
|
||
return customerRepository | ||
.save(customerWithEvents.result) | ||
.flatMap(customer -> domainEventPublisher.publish(Customer.class, customer.getId(), customerWithEvents.events).collectList().map(messages -> customer)) |
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.
This line is rather long
...
.collectList()
.map(message
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 will fix
import java.math.BigDecimal; | ||
|
||
@Table("reserved_credit") | ||
public class ReservedCredit { |
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.
This should be called CreditReservation and the table credit_reservation(s)
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 will fix
return id; | ||
} | ||
|
||
public void setId(Long id) { |
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.
Are setters needed?
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.
no
} | ||
|
||
@RequestMapping(value = "/events/orderserviceevents/io.eventuate.examples.tram.ordersandcustomers.orders.domain.Order/{aggregateId}/io.eventuate.examples.tram.ordersandcustomers.orders.domain.events.OrderCreatedEvent/{eventId}", method = RequestMethod.POST) | ||
public Mono<Void> handleOrderCreatedEvent(@RequestBody OrderCreatedEvent event, @PathVariable("aggregateId") String aggregateId) { |
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.
What support is there for handling duplicate events?
e.g. what would happen if the OrderCreatedEvent was delivered twice?
BTW If the primary key of a CreditReservation was (customerId, orderId) this might help
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.
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 it used in the Customer Service
?
If so what about the reactive vs. non reactive transaction issue?
This also means that the event handlers should not manage their own transactions.
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 it used in the Customer Service
?
If so what about the reactive vs. non reactive transaction issue?
This also means that the event handlers should not manage their own transactions.
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.
Chris, I do not understand. Http proxy delivers events to customer service, if event is duplicated it is not delivered to customer service.
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.
but r2dc does not support embedding
Spring Data R2DBC repositories might not support composite primary keys but r2dbc is a reactive library for executing SQL statements. The SQL statements can do what ever you want. This is some very simple database access code for loading/saving CreditReservations.
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.
So, I need to keep spring data for customer, but replace it by sql for reservations, right?
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.
So, I need to keep spring data for customer, but replace it by sql for reservations, right?
What are the options and their respective trade-offs. Which one do you think is best?
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.
Other option is to completely remove spring data. it has only one advantage: more consistent approach. However, I think it is ok to use simple high-level approach where it is possible and lower-level sql where it is not.
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.
Please file an issue to remove pointless duplicate key detection from the HTTP proxy.
Done: eventuate-tram/eventuate-tram-messaging-http#13
|
||
public Mono<Void> reserveCredit(long orderId, long customerId, Money orderTotal) { | ||
|
||
Mono<Customer> possibleCustomer = customerRepository.findById(customerId); |
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.
This doesn't handle the scenario where there are two simultaneously attempts to reserve credit.
Both will read existing and potentially exceed the available credit.
This might be a bug in the existing customer service BTW. The solution would be to implement optimistic locking in the Customer.
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.
As I know, optimistic locking works only with single row, but we are reserving credit using separate table. We can use Serializable isolation level. Or probably modify customer to hold a sum of reservations and if insert row to credit_reservation and update the sum in the same transaction, in case if there is concurrent modification, insert will be rolled back together with customer changes. In theory. I need to check docs to recall details about optimistic locking and write a test. Also there could be other useful lock types, for example postgres has Advisory Locks (https://www.postgresql.org/docs/9.1/explicit-locking.html), it could be something similar for mysql, but it definitely would complicate the code.
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.
implement optimistic locking in the Customer
The Customer has a version column, which is incremented whenever a CreditReservation is inserted/deleted.
e.g.
- Load Customer to get current version
- Insert/delete Credit Reservation
update Customer set version = version + 1 where id = ? and version = <old version>
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.
and if this update will return 0 updated rows, then rollback, right?
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. Rollback and retry.
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.
Got it, thank you
return customerRepository | ||
.save(customerWithEvents.result) | ||
.flatMap(customer -> domainEventPublisher.publish(Customer.class, customer.getId(), customerWithEvents.events).collectList().map(messages -> customer)) | ||
.as(transactionalOperator::transactional); |
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 code should use @Transactional
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.
Chris, as we discussed there are problems with that annotation, and I thought that we decided to use transnational operator
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 docs say that @transactional, which is the standard spring mechanism, should work for reactive. Correct?
Therefore, if it's not working in the example application then either the application is incorrect or there is a bug in spring.
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.
ok, I will research
No description provided.