Possible bug in DCAwareRoundRobinPolicy

[dkropachev]

Code under question is in the DCAwareRoundRobinPolicy.distance:

python-driver/cassandra/policies.py

Lines 257 to 264 in 1c3cff8

	dc_hosts = self._dc_live_hosts.get(dc)
	if not dc_hosts:
	return HostDistance.IGNORED
	if host in list(dc_hosts)[:self.used_hosts_per_remote_dc]:
	return HostDistance.REMOTE
	else:
	return HostDistance.IGNORED

This peace of the code creates a split in DCAwareRoundRobinPolicy.distance behavior for case when used_hosts_per_remote_dc is not empty:

1. if node was added to the policy and remote dc bucket does not contain more than used_hosts_per_remote_dc it will return HostDistance.REMOTE
2. Otherwise it will return HostDistance.IGNORE

Which means that if code uses .distance before on_add it will yield wrong result if this node fits into used_hosts_per_remote_dc.
Now you should always consider it when using lbp.distance.

Just one exampe that I managed to eyeball is in Cluster.on_add:

python-driver/cassandra/cluster.py

Lines 2067 to 2089 in 1c3cff8

	def on_add(self, host, refresh_nodes=True):
	if self.is_shutdown:
	return
	log.debug("Handling new host %r and notifying listeners", host)
	distance = self.profile_manager.distance(host)
	if distance != HostDistance.IGNORED:
	self._prepare_all_queries(host)
	log.debug("Done preparing queries for new host %r", host)
	self.profile_manager.on_add(host)
	self.control_connection.on_add(host, refresh_nodes)
	if distance == HostDistance.IGNORED:
	log.debug("Not adding connection pool for new host %r because the "
	"load balancing policy has marked it as IGNORED", host)
	self._finalize_add(host, set_up=False)
	return
	futures_lock = Lock()
	futures_results = []
	futures = set()

So it won't finalize properly for node from remote dc and won't run self._prepare_all_queries(host) for it.
Just becase it is doing:

        distance = self.profile_manager.distance(host)
        if distance != HostDistance.IGNORED:
            self._prepare_all_queries(host)
            log.debug("Done preparing queries for new host %r", host)

        self.profile_manager.on_add(host)

Instead of

        self.profile_manager.on_add(host)
        distance = self.profile_manager.distance(host)
        if distance != HostDistance.IGNORED:
            self._prepare_all_queries(host)
            log.debug("Done preparing queries for new host %r", host)

[dkropachev]

As I understand it was done to signal upper logic to connect only to a certain number of remote dc nodes.

My proposal, instead of going around the code and fixing distance usage to make sure that everywhere it is used to decide whether node is worth of connecting to or not, to delegate this functionality to another method on lbp like should_connect(host):bool and use it there for that purpose, removing self._dc_live_hosts from distance calculation.

[Lorak-mmk]

Hmm... by looking at the code it looks like you are right.
But if that were the case, then the driver would never connect to nodes outside the current DC, right?
I don't think that's the case, so maybe we are somehow misunderstanding the code - or it is the case and somehow no one noticed and no test checked this?
We should check if that really is the case or if we misunderstood something before trying to fix it.

If we were to fix this: I'd be strongly against other method. There should be only one (or maybe a few, but a low number) paths were nodes are added to policy. Those should be fixed (which will improve the driver and possibly uncover other bugs by reading code) instead of complicating code.