Ml/new control

[Michal-Leszczynski]

No description provided.

[mmatczuk]

ATM it's quite a mess.

It looks like you are trying to put too much things in one chunk of work and you got lost in your own balagan.

[mmatczuk]

All that session stuff is not needed here.

Let's just add the Cluster + events handling, it seems that the control conn can be just Conn.
We would send queries to control from Cluster.

Please do think about exported vs not exported fields.
Please respect object boundaries!!

[mmatczuk]

Should't this be called Close?

[mmatczuk]

Is there StartEventHandling

[mmatczuk]

@Michal-Leszczynski there is a fundamental BUG(s) here!!

• handleEvents is not prepared for this to close
• you are closing something you do not own
• conn will panic when you close it

[Michal-Leszczynski]

That's true and I actually spotted this bug because of TestClusterIntegration!
I already fixed it, but forgot to delete this useless function. As in the current form, closing control connection makes handling events loop terminate.

[mmatczuk]

See the final statement #130 (review)

[mmatczuk]

Left some comments inline.

One more general comment use if err := instead of if err = .

There is major flaw in the closing workflow.

Namely if c.control.Close() there is a go routine leak and calling StopEventHandling does not really stop anything can can cause panic when conn tries to handle events.

	if err := c.refreshTopology(); err != nil {
		log.Printf("failed to refresh topology: %s", err.Error())
		c.control.Close()

My suggestion

• cluster should have Close() function that would effectively close everything - all the Stop this stop that should not be exported or even removed
• cluster::events should be created in NewCluster
• cluster::events should be passed to Conn::registerEvents as input parameter - it's fully operated by cluster not conn - better idea later
• connReader need special handling of event stream because putting to cluster::events can block
• it would be better to pass a function to Conn::registerEvents that would be called on EVENT instead of registering a handler like in normal request sending
• with that there is a real full control of cluster::events in Cluster itself
• when you put to cluster::events it should be done via select with default namely if event processing is too slow it allows to DROP the event log it and DO NOT BLOCK connReader

Otherwise good piece of code.

Looking forward to next version.

[Michal-Leszczynski]

Im not sure whether it's worth to worry about blocking when putting response into cluster::events channel. I would assume that events are rarely received and the chance of blocking the channel is really small (we can set channel size to 1024 and then I would imagine that's pretty much impossible to block it in regular conditions).

But passing cluster::events down to connection and resigning from registering handler seems like a good idea and I will proceed with this approach (and with special event handling in connReader as well).

[Michal-Leszczynski]

Currently we have to separate goroutines in cluster: loop and handleEvents, but for now handleEvents only delegates work to loop or does some minor changes in cluster so it does not seem like it's that important?
In the future we will need to request some metadata from the cluster (also connected to schema change event) and we would like to refresh this data in terms of error or encountering some unknown tables or keyspaces, but this task does not really fall under handleEvent jurisdiction.

My idea is to have another loop responsible for handling refresh metadata requests, just like the loop responsible for handling topology refresh requests. In this scenario handling events is done just by passing some function (which will make some minor changes in cluster or delegate work directly to one of those two loops) to register events.

Another general problem arises from the fact, that if we have two goroutines and each of them wants to reopen control connection in case of error, we have to synchronize it. I see three possible solutions:

• some mutex guarding connection
• making two connections, one for each loop
• making one loop handle reopen control connection request (then one loop will be delegating this work to another)
  the last one may seem a little out of place, but is also the least problematic one.

I will try to read more rust to have a better perspective, but I would like to hear your opinion on this approach.

[Michal-Leszczynski]

@mmatczuk
I added new ml/delete_event_loop PR to demonstrate my idea.

[mmatczuk]

@Michal-Leszczynski

Regarding the passing cluster::events

• note that the issue of blocking exists and it needs to be sent with select
• please do not make the buffer too deep

Regarding go routines, it is best to have just one (the loop) function and use select to read from many channels.
There is not need for any synchronization then, it's synchronized by message passing.

[mmatczuk]

I had a look at the commit with the new approach.

• You can use c.handleEvent as input, there is no need to necessarily return the new function
• I like the idea with type of action because there is a single channel then - the code is easier
• Yet still Conn and Cluster should communicate by message passing

[mmatczuk]

Overall I think that many channels and single loop is more idiomatic and easier to maintain.

[Michal-Leszczynski]

With many channels we can set their buff length to 1, so that multiples of the same request (like refresh topology) will be handled by just one refresh topology action.

[mmatczuk]

That is correct.

[Michal-Leszczynski]

Corrected, squashed and rebased onto main version is on ml/delete_evet_loop PR. Can we merge it from there?

[mmatczuk]

Yes.