Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/cluster.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,19 @@ void Cluster::notify_or_record(const ClusterEvent& event) {
}
}

// TODO: This function takes a callback because it's called from both the host up and host add handlers.
// The usage there is to send appropriate notifications to all other nodes whether query prep succeeds or
// not. But that processing isn't really part of what _this_ function is trying to accomplish; that
// responsibility really relies with the host up or host add handler itself.
//
// A more robust implementation would be to implement the host up and host add handlers as a combined
// sequence of ops and then wait on that whole sequence to either complete or fail. That's a fairly
// significant change to the current impl, though, so for now just moving the notifications into
// the host up/add handlers will be sufficient.
//
// Why does any of this matter? Originally we were triggering this callback on socket close even when
// we were closing a connection. This link has already been severed (see CASSCPP-3) but that effort
// exposed the unnecessary entanglement described above.
bool Cluster::prepare_host(const Host::Ptr& host, const PrepareHostHandler::Callback& callback) {
if (connection_ && settings_.prepare_on_up_or_add_host) {
PrepareHostHandler::Ptr prepare_host_handler(
Expand Down
2 changes: 1 addition & 1 deletion src/connection.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class ConnectionListener {
*
* @param connection The closing connection.
*/
virtual void on_close(Connection* connection) = 0;
virtual void on_close(Connection* connection) {}
};

/**
Expand Down
2 changes: 0 additions & 2 deletions src/prepare_host_handler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ void PrepareHostHandler::prepare(uv_loop_t* loop, const ConnectionSettings& sett
}

void PrepareHostHandler::on_close(Connection* connection) {
callback_(this);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long-term the better answer is probably to not make HOST_UP/HOST_READY/HOST_ADD event distribution a function of the callback in use here. Those ops should be handled by logic in the relevant functions of cluster.cpp, specifically as some kind of "after" action from the prepare op. But this fix allows us to address the immediate problem without having to refactor the entirety of message delivery.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing the context. Shall we comment the todo in the code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I've added a TODO in the code.

I'm not sure it needs an actual ticket yet; if feels like a fairly big refactoring to take on and the change in this PR should sever the problematic link anyway so we should have a short-term fix now.


dec_ref(); // The event loop is done with this handler
}

Expand Down