improve Javadoc for VertxInstance stuff #1143
Conversation
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| /** | |||
| * An implementation of {@link VertxInstance} which allows the client | |||
| * to provide an instance of {@link Vertx} whose lifecyle is managed | |||
| * to provide an instance of {@link Vertx} whose lifecycle is managed | |||
There was a problem hiding this comment.
It's not part of your changes, but a few lines after this it states:
Hibernate will destroy the {@code Vertx} instance on shutdown.
This is not true though, isn't it?
a ProvidedVertxInstance won't do anything to the Vert.x instance when the factory is closed
There was a problem hiding this comment.
I think it's copied from DefaultVertxInstance, since you are alredy updating this, could you fix this too, please?
| needed. But if your program requires control over how the Vert.x instance is | ||
| created, or how it's obtained, you can override the default implementation and | ||
| provide your own `VertxInstance`. Let's consider this example: | ||
| of Vert.x when there is no instance associated with the calling thread. The |
There was a problem hiding this comment.
when there is no instance associated with the calling thread
Technically, it also defines how Hibernate Reactive obtains Vert.x even if there is one associated to the Thread.
Isn't this a bit misleading?
There was a problem hiding this comment.
Ummmmm. Hrrrrmmmm. Yeah, right. See there's a bit of a slightly confusing interplay between VertxContext and VertxInstance here. TBH I've never completely understood the significance of:
public void execute(Runnable runnable) {
io.vertx.core.Context context = vertxInstance.getVertx().getOrCreateContext();
if ( Vertx.currentContext() == context ) {
runnable.run();
}
else {
context.runOnContext( x -> runnable.run() );
}
}Like .... why don't we just use the Vertx.currentContext() if there is one? Why do we insist on it being the one coming from "our" Vert.x?
I mean, I know why we do it that way: because that's what @cescoffier and @vietj told us to do, but I think I need to understand it better.
There was a problem hiding this comment.
- When I talk about "an instance of Vert.x", I mean
io.vertx.core.Vertx. - When I talk about "a Vert.x context", I mean
io.vertx.core.Context.
So, DefaultVertxInstance is responsible for 1, org.hibernate.reactive.context.impl.VertxContext is responsible for 2.
But the VertxInstance service is always called when the factory is started (with or without an existing Vert.x instance), that's why I said that the line when there is no instance associated with the calling thread. doesn't seem correct (it implies that it gets called only when there is no Vert.x instance).
There was a problem hiding this comment.
Like .... why don't we just use the Vertx.currentContext() if there is one? Why do we insist on it being the one coming from "our" Vert.x?
Doesn't it makes sure that 1. we are running in a context (it will create a new one if one doesn't exist) and 2. an existing session run the operations always in the same context and that should imply the same thread (this is the part where I'm never sure).
There was a problem hiding this comment.
I think I need to think more about this
There was a problem hiding this comment.
I think I need to think more about this
Right, me too.
And I think perhaps we should wait until @Sanne gets back and discuss it thoroughly.
There was a problem hiding this comment.
Doesn't it makes sure that 1. we are running in a contex
I remember in the beginning we had a lot of issues with operations not running in the right context when using Quarkus.
| created, or how it's obtained, you can override the default implementation and | ||
| provide your own `VertxInstance`. Let's consider this example: | ||
| of Vert.x when there is no instance associated with the calling thread. The | ||
| default implementation just creates one the first time it's needed. But if your |
There was a problem hiding this comment.
Doesn't it create one when the service is started? So when the SessionFactory is started
There was a problem hiding this comment.
True. So then it's sort-of a bigger problem than I thought.
But default we go creating an instance of Vert.x even if we don't know we're going to need it.
| program requires control over how the Vert.x instance is created, or how it's | ||
| obtained, you can override the default implementation and provide your own | ||
| `VertxInstance`. | ||
|
|
There was a problem hiding this comment.
I'm not sure about anything written after this point.
I think there are only two things to mention:
- Hibernate Reactive will bind to the current Vertx instance if the factory is created inside an existing context (and will log a message about it)
- Hibernate Reactive will create a new instance if none is detected when the factory is created
The user needs to do something only if this is not the behaviour they want.
There are many way to create a factory inside a context without using runOnContext.
There was a problem hiding this comment.
Ah sorry, I mean it logs the message when a new Vertx instance is created
There was a problem hiding this comment.
We should also mention that when Hibernate Reactive creates a new instance of Vert.x, it will also shut it down when the factory gets closed.
There was a problem hiding this comment.
See what I documented is how I actually want it to work.
But you're right: it doesn't work like that today.
There was a problem hiding this comment.
how I actually want it to work.
Well, naively.
| IMPORTANT: If your program starts Vert.x externally to Hibernate Reactive, and | ||
| you don't tell Hibernate Reactive how to obtain this instance of Vert.x by | ||
| specifying an implementation of `VertxInstance`, you might end up with multiple | ||
| instances of `Vertx` in your program, resulting in confusing error messages. |
There was a problem hiding this comment.
Isn't this a bug from our side? Shouldn't everything work anyway even if we start a different Vert.x instance?
There was a problem hiding this comment.
Well, I would expect it to work if one uses .withSession.
There was a problem hiding this comment.
If the program already has a Vertx, and we go and create one, then that's simply wrong, I would have thought.
There was a problem hiding this comment.
I remember reading that one could use these scenario if they want different event busses for different groups of Verticles.
I might have dreamt it.
But, even if it's unlikely, I'm not sure it's wrong
There was a problem hiding this comment.
Found it! It was in the Vert.x documentation:
Most applications will only need a single Vert.x instance, but it’s possible to create multiple Vert.x instances if you require, for example, isolation between the event bus or different groups of servers and clients.
No description provided.