Force driver to use Vert.x event loop #4

Sammers21 · 2018-08-17T07:45:26Z

Fixes #3

…lient into integrate_driver_and_vertx_threads

Sammers21 · 2018-08-17T07:49:05Z

@vietj , @tsegismont , can you review?

vietj · 2018-08-17T09:20:57Z

so if we use this event loop, what are we doing a on cassandra event loop callback ? can you point out code ?

Sammers21 · 2018-08-17T13:45:09Z

You mean in which thread we are, when a callback is called?

Sammers21 · 2018-08-17T13:47:01Z

This is how driver's bootstrap is configured: https://github.com/datastax/java-driver/blob/3.x/driver-core/src/main/java/com/datastax/driver/core/Connection.java#L1032

vietj · 2018-08-17T14:39:20Z

I was actually referring to the code called back from Cassandra like:

vertxExecuteFuture.setHandler(executionResult -> {
   // vertx code
});

in the final callback to the user code:

do we check that the event loop thread is the same
the vertx context is the same as well

do you use ContextInternal#executeFromIO before calling user code ?

vietj · 2018-08-17T14:39:54Z

cf this : https://github.com/vietj/vertx-materials/blob/master/src/main/asciidoc/output/Integrating_Netty.adoc

Sammers21 · 2018-08-26T09:53:43Z

Ah, I got it. Sure, we need to call ContextInternal#executeFromIO before invoking application code

Sammers21 · 2018-08-26T09:55:35Z

I also think that driver should use acceptor event loop because we can't specify one for networking and one for calling handlers

Sammers21 · 2018-08-26T09:56:13Z

@vietj , can you review, please?

Sammers21 · 2018-09-05T08:10:15Z

@vietj , ping

vietj · 2018-09-05T09:17:55Z

I think we should have tests with callbacks that:

1/ check the context is the right one
2/ throwing an exception in the handler is actually reported on the Context#exceptionHandler

Sammers21 · 2018-09-15T14:55:55Z

@vietj , done. Can you review one more time, please?

vietj · 2018-09-20T06:40:35Z

src/main/java/io/vertx/cassandra/impl/CassandraClientImpl.java

-        } else {
-          if (resultHandler != null) {
-            resultHandler.handle(Future.failedFuture(executionResult.cause()));
+      vertxExecuteFuture.setHandler(executionResult ->


you can simplify to :

vertxExecuteFuture.setHandler(executionResult -> { if (resultHandler != null) { ctx.executeFromIO(executionResult, resultHandler); } });

vietj · 2018-09-20T06:42:05Z

there are simplifications you can do in callbacks, there is a method on ContextInternal that allows to run executeFromIO using an Handler<T> that takes a T as parameter you can use. Also re-creating a Future from a Future is not necessary. Take a look.

Sammers21 · 2018-09-20T13:17:16Z

@vietj, can you review once again?

Sammers21 · 2018-09-24T13:09:57Z

@tsegismont , can you review, please?

tsegismont

@Sammers21 the changes look good to me, but it would be better to create a dedicated test for context checking.

tsegismont · 2018-10-02T20:25:47Z

src/test/java/io/vertx/cassandra/ConnectionTest.java

@@ -50,10 +50,12 @@ private void connectAndDisconnect(TestContext context, CassandraClientOptions op
    );
    Async async = context.async(2);
    cassandraClient.connect("system", connectEvent -> {
+      checkContext(context);


I don't believe it is necessary to do this in all the tests in the test suite. Instead, we could have a test dedicated to context checking (with different use cases for shared/non shared client)

I don't believe it is necessary to do this in all the tests in the test suite. Instead, we could have a test dedicated to context checking

Agree.

with different use cases for shared/non shared client

Why do we need to have context checks for both of them?

…ent into integrate_driver_and_vertx_threads

Sammers21 · 2018-10-05T11:08:17Z

It seems that there is something wrong with this approach. We are getting IllegalStateExceptions:

java.lang.IllegalStateException: Uh oh! Event loop context executing with wrong thread! Expected null got Thread[main,5,main]
	at io.vertx.core.impl.ContextImpl.executeTask(ContextImpl.java:318)
	at io.vertx.core.impl.ContextImpl.executeFromIO(ContextImpl.java:188)
	at io.vertx.core.impl.EventLoopContext.executeFromIO(EventLoopContext.java:23)
	at io.vertx.cassandra.impl.CassandraRowStreamImpl.tryToTriggerEndOfTheStream(CassandraRowStreamImpl.java:134)
	at io.vertx.cassandra.impl.CassandraRowStreamImpl.endHandler(CassandraRowStreamImpl.java:82)
	at io.vertx.cassandra.StreamingTest.lambda$emptyStream$11(StreamingTest.java:95)

java.lang.IllegalStateException: Uh oh! Event loop context executing with wrong thread! Expected null got Thread[main,5,main]
	at io.vertx.core.impl.ContextImpl.executeTask(ContextImpl.java:318)
	at io.vertx.core.impl.ContextImpl.executeFromIO(ContextImpl.java:188)
	at io.vertx.core.impl.EventLoopContext.executeFromIO(EventLoopContext.java:23)
	at io.vertx.core.impl.ContextImpl.executeFromIO(ContextImpl.java:179)
	at io.vertx.core.impl.EventLoopContext.executeFromIO(EventLoopContext.java:23)
	at io.vertx.cassandra.impl.CassandraClientImpl.lambda$null$15(CassandraClientImpl.java:253)
	at io.vertx.core.impl.FutureImpl.setHandler(FutureImpl.java:79)
	at io.vertx.cassandra.impl.CassandraClientImpl.lambda$disconnect$16(CassandraClientImpl.java:253)
	at io.vertx.cassandra.impl.CassandraClientImpl.executeWithSession(CassandraClientImpl.java:262)
	at io.vertx.cassandra.impl.CassandraClientImpl.disconnect(CassandraClientImpl.java:251)
	at io.vertx.cassandra.ExecutionTest.lambda$simpleReleaseVersionSelect$8(ExecutionTest.java:124)

https://travis-ci.org/vert-x3/vertx-cassandra-client/jobs/437563597

Sammers21 · 2018-10-05T11:09:50Z

@vietj , @tsegismont do you have any ideas on what is wrong?

Sammers21 · 2018-10-05T11:12:24Z

I am also not able to reproduce it on my on my laptop (tried 10 times). looks weird.

tsegismont · 2018-10-08T07:24:32Z

@Sammers21 looking

tsegismont · 2018-10-08T07:35:52Z

I can reproduce on my box with a vertx-unit repeat rule. Working on the fix.

tsegismont · 2018-10-08T09:12:57Z

@Sammers21 the issue is here:

vertx-cassandra-client/src/main/java/io/vertx/cassandra/impl/CassandraClientImpl.java

Lines 160 to 164 in 7f7fb20

    
           sessionFuture.setHandler(executionResult -> { 
        
             if (connectHandler != null) { 
        
               context.executeFromIO(executionResult, connectHandler); 
        
             } 
        
           });

When sessionFuture.setHandler is invoked, it is possible that the future is already completed. Then the calling thread will invoke the handler. In our case, the calling thread is the Java main thread.

The issue is not specific to this PR, it is simply brought forward because executeFromIO does thread checks.

tsegismont · 2018-10-08T12:26:48Z

@Sammers21 I believe the acceptor event loop group shouldn't be used

vertx-cassandra-client/src/main/java/io/vertx/cassandra/impl/CassandraClientImpl.java

Lines 280 to 283 in 7f7fb20

    
           @Override 
        
           public EventLoopGroup eventLoopGroup(ThreadFactory threadFactory) { 
        
             return vertx.getAcceptorEventLoopGroup(); 
        
           }

It's a single thread group and we don't all Datastax connections to handled by a single event loop

tsegismont · 2018-10-08T12:30:05Z

@Sammers21 I overlooked key changes the first time I reviewed this PR. For this to work we must be sure that the Datastax driver callbacks will always be invoked on the connection event loop. I don't believe this is true.

vietj · 2018-10-08T14:18:46Z

you can use EventExecutor#inEventLoop() to check wether yo uare in event loop or not. Julien

…

On 8 Oct 2018, at 14:30, Thomas Segismont ***@***.***> wrote: @Sammers21 <https://github.com/Sammers21> I overlooked key changes the first time I reviewed this PR. For this to work we must be sure that the Datastax driver callbacks will always be invoked on the connection event loop. I don't believe this is true. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANxipk4atFTgwP_3nMrCt0naMwVhWGZks5ui0VTgaJpZM4WBHjM>.

tsegismont · 2018-10-08T15:38:16Z

@vietj right. But my point was that chances are low that the event loop running the driver connection is the same as the event loop running the calling context.

In the end, to make this right, we'll add a lot of complexity for a small benefit.

@Sammers21 @vietj I just submitted #9 and I would rather close this one

Sammers21 · 2018-10-09T08:58:09Z

@tsegismont, the #9 PR is fine to me. But, can we make the problem more explicit? A Vert.x instance maintain the acceptor event loop, which is a single-threaded event loop. The event loop should be used for network operations and never be used for executing user code. Right?

Why can't we use the acceptor event loop for network operations with Cassandra and delegate "calling handlers" to a context related event loop, so user code(inside one verticle) will always be executed in the same context.

tsegismont · 2018-10-09T13:11:26Z

A Vert.x instance maintain the acceptor event loop, which is a single-threaded event loop.

The acceptorEventLoopGroup is a group of a single event loop:

https://github.com/eclipse-vertx/vert.x/blob/9cafc3673424722c463d3e9d3754821f1bb86484/src/main/java/io/vertx/core/impl/VertxImpl.java#L153-L155

Its purpose is to avoid having connections (HTTP servers, net servers) rejected because all the event loops used for verticles are too busy. It should not be reused in the Cassandra client to create event loop for driver connections.

vertx-cassandra-client/src/main/java/io/vertx/cassandra/impl/CassandraClientImpl.java

Lines 280 to 283 in 7f7fb20

    
           @Override 
        
           public EventLoopGroup eventLoopGroup(ThreadFactory threadFactory) { 
        
             return vertx.getAcceptorEventLoopGroup(); 
        
           }

So the code above is wrong. If anything, we should use VertxInternal#getEventLoopGroup

The event loop should be used for network operations and never be used for executing user code. Right?

Not sure what you mean.

Why can't we use the acceptor event loop for network operations with Cassandra and delegate "calling handlers" to a context related event loop, so user code(inside one verticle) will always be executed in the same context.

I believe I answered the question related to acceptor. As for using Vert.x event loops, it sounded to me like a good idea in the beginning, but taking a closer look I can only see problems but not much benefits.

The problems are:

drivers connections are handled by a single event loop
there is no guarantee that a verticle using our client will always be assigned the same driver connection

Therefore, the benefits (a bit less threads) are not going to outweigh the added complexity.

tsegismont · 2018-10-10T09:56:59Z

@Sammers21 I'll go ahead and merge #9

We can either close #3, or keep it open and try to demonstrate the benefits with a benchmark?

Sammers21 · 2018-10-10T10:02:05Z

@tsegismont , I we can close this PR, and keep the #3 issue open.

Sammers21 · 2018-10-10T10:04:04Z

try to demonstrate the benefits with a benchmark

@tsegismont, yes I think we need to measure performance benefits

Sammers21 added 4 commits June 12, 2018 14:15

force driver to use Vert.x event loop - fixes #21

6dcb2d9

move custom NettyOptions to to an inner class

2b653b9

Merge branch 'initial-work' of github.com:Sammers21/vertx-cassandra-c…

0c98cb5

…lient into integrate_driver_and_vertx_threads

Merge branch 'initial-work' of github.com:Sammers21/vertx-cassandra-c…

00dec39

…lient into integrate_driver_and_vertx_threads

Sammers21 added the enhancement New feature or request label Aug 17, 2018

Sammers21 self-assigned this Aug 17, 2018

Sammers21 requested review from vietj and tsegismont August 17, 2018 07:45

Sammers21 added 2 commits August 17, 2018 10:47

Merge branch 'initial-work' into integrate_driver_and_vertx_threads

9b26032

rearrange imports

a085e3c

Sammers21 added a commit that referenced this pull request Aug 17, 2018

add test on shared client - fixes #4

e107ee9

Sammers21 mentioned this pull request Aug 17, 2018

Force driver to use Vert.x event loop Sammers21/vertx-cassandra-client#22

Closed

call context.executeFromIO before executing application code

af72f39

check that all handlers is called in the same context

6bd5742

vietj reviewed Sep 20, 2018

View reviewed changes

simplify executeFromIO usage

ac5a2f5

tsegismont requested changes Oct 2, 2018

View reviewed changes

tsegismont mentioned this pull request Oct 2, 2018

Update CassandraRowStream with Vert.x 3.6.0 ReadStream changes #7

Merged

Sammers21 added 3 commits October 5, 2018 13:35

Merge branch 'initial-work' of github.com:vert-x3/vertx-cassandra-cli…

19e4e9e

…ent into integrate_driver_and_vertx_threads

CassandraRowStream handler calls should be in the same Vert.x context

f81fe98

remove context checks all test suits and add dedicated test suit

4dad269

Sammers21 added 3 commits October 5, 2018 14:23

simplify executeFromIO call

5d3def5

don't check context when handler called from the non vert.x thread

7b85c2e

rollback * import changes

7f7fb20

Sammers21 closed this Oct 10, 2018

Force driver to use Vert.x event loop #4

Force driver to use Vert.x event loop #4

Conversation

Sammers21 commented Aug 17, 2018 • edited Loading

Sammers21 commented Aug 17, 2018

vietj commented Aug 17, 2018

Sammers21 commented Aug 17, 2018 • edited Loading

Sammers21 commented Aug 17, 2018

vietj commented Aug 17, 2018

vietj commented Aug 17, 2018

Sammers21 commented Aug 26, 2018

Sammers21 commented Aug 26, 2018

Sammers21 commented Aug 26, 2018

Sammers21 commented Sep 5, 2018

vietj commented Sep 5, 2018

Sammers21 commented Sep 15, 2018

vietj Sep 20, 2018

Choose a reason for hiding this comment

Sammers21 Sep 20, 2018

Choose a reason for hiding this comment

vietj commented Sep 20, 2018 • edited Loading

Sammers21 commented Sep 20, 2018

Sammers21 commented Sep 24, 2018

tsegismont left a comment

Choose a reason for hiding this comment

tsegismont Oct 2, 2018

Choose a reason for hiding this comment

Sammers21 Oct 5, 2018

Choose a reason for hiding this comment

Sammers21 commented Oct 5, 2018 • edited Loading

Sammers21 commented Oct 5, 2018

Sammers21 commented Oct 5, 2018 • edited Loading

tsegismont commented Oct 8, 2018

tsegismont commented Oct 8, 2018

tsegismont commented Oct 8, 2018

tsegismont commented Oct 8, 2018

tsegismont commented Oct 8, 2018

vietj commented Oct 8, 2018 via email

tsegismont commented Oct 8, 2018

Sammers21 commented Oct 9, 2018

tsegismont commented Oct 9, 2018 • edited Loading

tsegismont commented Oct 10, 2018

Sammers21 commented Oct 10, 2018

Sammers21 commented Oct 10, 2018

Sammers21 commented Aug 17, 2018 •

edited

Loading

Sammers21 commented Aug 17, 2018 •

edited

Loading

vietj commented Sep 20, 2018 •

edited

Loading

Sammers21 commented Oct 5, 2018 •

edited

Loading

Sammers21 commented Oct 5, 2018 •

edited

Loading

tsegismont commented Oct 9, 2018 •

edited

Loading