feat: add a ImmutableBuffer #69
Conversation
kristian
force-pushed
the
feature/immutablebuffer
branch
29 times, most recently
from
d9ae38e
to
853ce11
Compare
After many CI related fixes, the NeonBeeTestBaseTest was the last test that did fail on the GitHub workflow, for a long thought inexplicable reason. The root cause finally identfied, revealed a series of yet unfound issues. The reason why the NeonBeeTestBaseTest failed in the first place, is that if a custom provideUserPrincipal method was provided in a test, the NeonBeeTestBase attempted to replace the ServerVerticle with a custom ServerVerticle that was invoking provideUserPrincipal to retrieve the user principal it should return. However due to an issue causing the port to already be blocked for the second deployment of ServerVerticle, the setUp of NeonBeeTestBase failed. Due to a bug in the setUp code, the CountDownLatch used to stop JUnit from invoking more `@BeforeEach` methods, before NeonBee was initialized, the thread was blocked forever and JUnit (as well as any built-in logic in the `@Timeout` annotation) was not able to finish the test and the execution stalled. This commit fixes all issues related to this behaviour by: - check all latches and make sure they ALWAYS use timeouts - have NeonBee not close Vert.x that had been supplied from the outside - make injecting a custom user principal more reslient to failure
kristian
force-pushed
the
feature/immutablebuffer
branch
2 times, most recently
from
d693e70
to
3ba2f09
Compare
kristian
force-pushed
the
feature/immutablebuffer
branch
2 times, most recently
from
038c249
to
b5c09a7
Compare
| @@ -154,7 +156,13 @@ public WatchVerticle(Path watchPath, long interval, TimeUnit unit, boolean paral | |||
| public void start(Promise<Void> startPromise) { | |||
| Vertx vertx = getVertx(); | |||
| if (NeonBee.get(vertx).getOptions().doNotWatchFiles()) { | |||
There was a problem hiding this comment.
In my opionion the we shouldn't check in WatchVerticle if the NeonBee option "doNotWatchFiles" ist set or not. It is a NeonBeeOption and should only change the behavior of NeonBee. If I want to deploy a Module, which contains a WatchVerticle I don't want that this NeonBee option prevents it.
We have 2 verticles that extends the WatchVerticle DeployerVerticle and ModelRefresher. If NeonBee option "doNotWatchFiles" is set, we simply shouldn't deploy this verticles.
There was a problem hiding this comment.
I see your point, however then we have a different behaviour in JobVerticle than we have in WatchVerticle. And if you don't want to have any files watched, that's what the NeonBee option is for. The question is: Should we "enforce" it, then I would say, adding it to the WatchVerticle is kind of how we did it in the JobVerticle as well, or should we not enforce it, then I would agree to your option. What do you think?
There was a problem hiding this comment.
Maybe its the name of the option. I thought we introduced the option "doNotWatchFiles" with the idea to stop NeonBee watching the folders "models" and "verticles". Or did we had another idea in mind?
If this was the idea, I wouldn't enforce it. I got your point with the similarity to "shouldDisableJobScheduling", but then maybe we simply should rename the option into "doNotWatchModules" and try to "remove" the models folder as soon as possible. Or rename it to "doNotWatchResources" or "doNotWatchModelsAndModules".
To be honest in my opinion "shouldDisableJobScheduling" is also not really required. I'd prefer disabling the "unwanted" JobVerticles via their config.
There was a problem hiding this comment.
Well both options are particularly useful in tests, because in many / most tests you don't want to have files watch nor you want job verticles, Thus the default option for tests has been set to "true / true" so no jobs are started and no files will be watched. Maybe we should move this debate to an issue instead to have it in our backlog and resolve this conversation?
There was a problem hiding this comment.
I don't know, because this change then introduces a behavior which maybe is unwanted and needs to be removed afterwards. On the other hand this PR is so big in the meanwhile, that it is hard to review. So maybe it makes sense to merge it now as it is and do the "removal" maybe later.
I like none of these options, so you can decide :)
There was a problem hiding this comment.
I would like to merge this change now :D if you permit! 👯
Then this beast and the whole chapter on non-running tests is hopefully and FINALLY closed!
kristian
force-pushed
the
feature/immutablebuffer
branch
5 times, most recently
from
9f18801
to
f527472
Compare
|
The change is now DONE. Can we merge, please? 👍 |
Allows efficient chaining of buffers, without having to create copies of them. Especially useful when transferring large data sets via the event bus.
kristian
force-pushed
the
feature/immutablebuffer
branch
from
f527472
to
d60b877
Compare
|
It's done! 👍 Thanks a lot to both of you! @gedack, feel free to rebase your other pull requests now, which should fix the voter for them finally! cheer |
ImmutableBuffer throws a UnsupportedOperationException for anything that writes to the buffer. This disadvantage is compensated by the advantage the ImmutableBuffer can be transfered over the local event bus very efficiently without beeing copied.