Closed heutelbeck closed 9 years ago
Yes, but actually this is by design and documented:
The Repository
interface provided by JCommonDomain requires that
aggregates queried do exist, see the javadoc on ofIdentity()
in
https://github.com/JEEventStore/JCommonDomain/blob/master/common/src/main/java/org/jeecqrs/common/domain/model/Repository.java.
So, querying for an aggregate that does not exist is considered a programming error.
To avoid this, one can check whether the aggregate root exists before loading the AR. We usually do this on a case by case basis (usually the command handler would know whether the command can be run against non-existing aggregate roots and how to handle both cases).
The rationale behind this contract on ofIdentity()
is that a NotFoundException
on ofIdentity()
would induce lots of try ... catch
boilerplate code in every single consumer of the Repository
interface, while in most cases that additional check is simply not required (commands are seldom issued against
non-existing aggregate roots). Thus, we prefer to use an additional exists()
in the rare cases where commands can be sent against non-existing aggregate roots.
This is a somewhat personal preference, of course. However, note that JEEventStore throws a checked exception org.jeeventstore.StreamNotFoundException
when the stream id does
not exist (see above stacktrace). This can of course be propagated to the repository accordingly. Thus, when a NotFoundException
is desired, it should be straight-forward to implement as custom repository based on the JCommonDomain implementation.
By the way, both JEEventStore and JEECQRS additionally provide a completely synchronous implementation of command bus, sagas and event handlers, such that a client can be sure all changes have been committed to the database when the commandBus.send()
returns. We usually use the synchronous implementations for small projects with few users, and the asynchronous model for "web scale" applications with many concurrent users.
Ok, I see. Still makes me cringe that the resulting error takes down other infrastructure componens (e.g. command bus) on the resulting error. To me this feels a bit undeterministic from the perspective of the client of the repository.
Also I do not really get the rationale behind the contract. Even if you return a NotFoundException to the client of the Repository interface, the client does not have to implement the try...catch boilerplate if they want the behaviour you have now (maybe just one block at most), but they have to opportunity to do so.
I currently do not see an advantage of not handling the exception gracefully.
It is actually really easy to change the behaviour. But it is also really akward. I had to:
protected abstract void loadFromStream(T obj, String streamId) throws NotFoundException;
in AbstractEventSourcingRepository
AbstractJEEventStoreARRepository
, AbstractEventSourcingRepository
, AbstractMultiTenancyEventStoreRepository
, MultiTenancyRepository
Just because this is down in the root class of the hierarcy. However this fixes the issue and gets rid of unchecked exceptions running havok in the application server basically taking down the entire infrastructure from one malformed request.
Just a suggestion. I can see that this would affect your client code at the moment because yes, there is at least one try...catch
or a throws
more needed in the client code. But the entire system becomes more stable from my point of view.
Yeah, I noticed the sync and async implementations. I usually prefer async implementations to get more reactive applications.
A small section on how to set up the application correctly would be great.
What do you mean with "takes down other infrastructure components"? If the command handler throws a RuntimeException
while handling the command asynchronously, the transaction fails and the application server will log an error. The above stacktrace seems to confirm this behaviour (e.g., logged EJBTransactionRolledbackException
with the internal root cause).
Additionally, the application server will try to re-execute the command at least once. This is because the commandBus is internally using @Timeout
for the asynchronous execution, and the Java EE 6 spec states:
If the transaction is rolled back, the container will call the @Timeout method at least one more time. http://docs.oracle.com/javaee/6/tutorial/doc/bnboy.html#bnbpd
Thus it might seem as if the infrastructure is failing, but actually it's just trying the command several times (I believe twice on JBoss/Wildfly) and each time logging that it could not execute the command in a row in a bold way with the full strack trace - which is desired, since async command handlers must be written such that they do not throw exceptions.
Subsequent commands or commands handled in parallel should not be affected.
Ah, I now understand what you meant with the NotFoundException
, thanks for elaborating.
We can declare a throw NotFoundException
in Repository#ofIdentity
, but if clients do not want a throwing ofIdentity
, they can simply subclass Repository
once in their project and remove the throw declaration.
Yes, you are right. It's much better to give clients the opportunity to handling this exception gracefully. I'm going to implement this.
recommendend/tested data sources and configurations (postgres, mysql,...). Currently I use Porstgres. Mysql was giving me some issues with the body fields of the event store.
Most problems are caused by the automatic creation of the schema due to known bugs in the ORM layer (namely Hibernate) w.r.t. large text fields (which affects the body field, as you probably have noticed). I therefore recommend manual schema creation, here are the schemas for MySQL and Postgres: https://github.com/JEEventStore/JEEventStore/tree/master/persistence-jpa/src/main/sql
JEEventStore runs automatic tests against MySQL, Postgres and Derby on Travis CI, but we have only used Postgres in production.
I will see what I can do about the documentation about setting up the application, but it will take some time. If you have questions regarding a specific setup, let me know, I can probably answer them much faster.
What do you mean with "takes down other infrastructure components"? If the command handler throws a RuntimeException while handling the command asynchronously, the transaction fails and the application server will log an error. The above stacktrace seems to confirm this behaviour (e.g., logged EJBTransactionRolledbackException with the internal root cause).
Ok, what happened in my case with a trivial test was, that I issued a "createAggregate" command, and before the command handler received the command, I issed the "ofIdentity" query to the repository.
This resulted in the unchecked exception as discussed, and the command was never received by the commandHandler.
However, I suspect this was an edge case because I did this in the @PostCronstruct method of a startup bean where I executed some experiments. So I think what happened here was, that the undhandled Exception terminated the application startup and shut it down before the command handler was called. So probably you are correct with being confused about the "takes down other infrastructure components", and this just happened because of this specific setup.
So this is likely not an issue.
Thanks for implementing the Exception thogh. Definitely nicer.
Context: A multi tenant aggregate root and its repository with a create command.
If I do a
and directly follow this with:
Due to the asynchronous nature of the process, the aggregate has not been created yet. The resulting exceptions cause the commandBus to break (maybe other things as well. I did not check beyond that) and the command is never delivered. This must not happen, but result in a graceful exception handling and should result in something like a NotFoundException.