-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add RackAwareRoundRobinPolicy
for host selection
#332
Add RackAwareRoundRobinPolicy
for host selection
#332
Conversation
@Lorak-mmk @fruch ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- few clarifications, and a bit more documenting the internal implementation
- I think adding integration test for this, should be very helpful, also as documentation on how to use it
- and documentation, the docstring of the class isn't enough, we should have an example how a user should be expected to set it up. (in the java implementation was all bunch of hidden assumptions, and it took us quite time to figure why c-s wasn't working as expected), in this case we don't have a stress tool readily available to test the same in SCT, so would recommend investing documentation a bit more.
cassandra/policies.py
Outdated
|
||
def populate(self, cluster, hosts): | ||
for (dc, rack), dc_hosts in groupby(hosts, lambda h: (self._dc(h), self._rack(h))): | ||
self._live_hosts[(dc, rack)] = tuple(set(dc_hosts)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get to situation we have duplicates hosts, passed to this function ? that a set
is needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
o.k., I was just wondering how we get to that situation.
as for the integration test with multi dc and multi-racks, I would recommend looking into similar test in dtest, and clone the needed ideas/helper functions from there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this new policy should be the default? I think you need to modify default_lbp_factory
function in cluster.py to do this (+ comment in class ExecutionProfile
's load_balancing_policy
attribute and perhaps some other comments). @mykaul wdyt?
In general please go over all occurences of DCAwareRoundRobinPolicy
in the driver code (and docs etc) and see if they need to be replaced or complemented by RackAwareRoundRobinPolicy
`used_hosts_per_remote_dc` controls how many nodes in | ||
each remote datacenter will have connections opened | ||
against them. In other words, `used_hosts_per_remote_dc` hosts | ||
will be considered :attr:`~.HostDistance.REMOTE` and the | ||
rest will be considered :attr:`~.HostDistance.IGNORED`. | ||
By default, all remote hosts are ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to think if there should be similar mechanism for racks, but I don't immediately see how would it work.
what's the reason ? i.e. it would be doing the exact same thing, since no parameters are passed to it.
again, don't know about replacing, but each occurence need to be understood, and figure if there's something todo with it. |
9092800
to
8e5cf99
Compare
cassandra/policies.py
Outdated
if not dc_hosts: | ||
return HostDistance.IGNORED | ||
|
||
if host in list(dc_hosts)[:self.used_hosts_per_remote_dc]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conversion still needs to copy whole list, right? Why do this weird tuple(set())
conversion in populate
instead of just holding a list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because every other policy uses set:
https://github.com/scylladb/python-driver/blob/master/cassandra/policies.py#L164
https://github.com/scylladb/python-driver/blob/master/cassandra/policies.py#L240
https://github.com/scylladb/python-driver/blob/master/cassandra/policies.py#L451
I just wanted to be in line with other policies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo we should understand why they are using set and not just following it.
Either there is a good reason, which should be documented, or there is not - then we could change it here and maybe in other policies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is no good reason
cassandra/policies.py
Outdated
# the dict can change, so get candidate DCs iterating over keys of a copy | ||
other_dcs = [dc for dc in self._dc_live_hosts.copy().keys() if dc != self.local_dc] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we cloning the whole dict here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why make a list and not just iterate generator directly?
8e5cf99
to
a45adb2
Compare
v2:
|
Yes, I believe it should be the default. What is the default in other drivers? |
@mykaul |
This sounds like what we should support as well. |
We can pass If we would want to replace all occurrences of |
Now that we decided to not do implicit dc / rack it makes no sense imo to make this policy default. Because without specifying dc / rack it won't do anything, and as @sylwiaszunejko mentioned specifying it is equivalent to using it as child policy to TokenAware. |
a45adb2
to
05e59a5
Compare
I added some kind of pattern for integration tests, but I am not really sure how the tests should look like. I could use some help with that @Lorak-mmk @fruch |
response = self.session.execute('SELECT * from system.local') | ||
queried_hosts.update(response.response_future.attempted_hosts) | ||
queried_hosts = set(host.address for host in queried_hosts) | ||
# Performe some checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use a similar approach as in shard aware tests, and use tracing to validate requests arrived at the correct nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would need to be done carefully because of heat weighed load balancing in Scylla - if it kicks in, then entries in tracing will be different than expected. I've run into this before with shard awareness tests.
05e59a5
to
bc88863
Compare
I am not sure what is going on with the CI. I cannot download full logs it keeps failing every time. Locally all tests pass |
I am still fighting with the CI, this part of the code in for host in islice(cycle(local_rack_live), pos, pos + len(local_rack_live)):
yield host Even though |
6fcc371
to
5cd5ecf
Compare
I changed this slice logic to just iterating through list and the issue stopped reproducing. @Lorak-mmk do you have any idea why? |
@Lorak-mmk ping |
It turns out that using import threading
import time
from itertools import cycle, islice
# Shared list among threads
local_rack_live = ['host1', 'host2', 'host3', 'host4', 'host5']
def remove_elements():
"""Thread function to remove elements from the list."""
while local_rack_live:
time.sleep(1) # Simulate some delay
removed_host = local_rack_live.pop(0)
print(f"Removed: {removed_host}")
def iterate_and_yield():
"""Thread function to iterate through the list and yield elements."""
pos = 0
while local_rack_live:
# Ensure pos is valid
pos = (pos % len(local_rack_live)) if local_rack_live else 0
for host in islice(cycle(local_rack_live), pos, pos + len(local_rack_live)):
print(f"Yielding from list: {local_rack_live} host: {host}")
time.sleep(1) # Simulate some delay
pos += 1
# Create threads
remove_thread = threading.Thread(target=remove_elements)
iterate_thread = threading.Thread(target=iterate_and_yield)
# Start threads
remove_thread.start()
iterate_thread.start()
# Wait for threads to complete
remove_thread.join()
iterate_thread.join() The output looks like that:
Clearly it doesn't work returning e.g. host1 when the list looks like [host4, host5]. |
2bdae06
to
3532186
Compare
I have to correct myself here. In other policies such as DCAwareRoundRobinPolicy hosts are stored in a tuple and when they are changes (host up/down) the value in the directory is reassigned so the tuple we get from the dict before the change is unaffected e.g. here: https://github.com/scylladb/python-driver/blob/master/cassandra/policies.py#L272. |
This approach makes sense - copy in on_up / on_down because those are rare so can be expensive, don't copy in make_query_plan because we want it to be fast. |
Maybe but it was not that easy to deduct from the code before. So I guess the optimal solution will be to return to using tuples and copy in on_up / on_down, do you agree? @Lorak-mmk |
I agree. Please also verify that other policies are implemented correctly in this regard. |
3532186
to
3ed0ad6
Compare
if host not in current_rack_hosts: | ||
self._live_hosts[(dc, rack)] = current_rack_hosts + (host, ) | ||
current_dc_hosts = self._dc_live_hosts.get(dc, ()) | ||
if host not in current_dc_hosts: | ||
self._dc_live_hosts[dc] = current_dc_hosts + (host, ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that DCAwarePolicy does the same thing. I wonder if it is possible for on_up
to be marked on already present host. It probably is, otherwise those checks wouldn't exist. Why would driver do this I have no idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure to be honest
33af849
to
ee99b31
Compare
ee99b31
to
c62665f
Compare
Added
RackAwareRoundRobinPolicy
to prefer replicas in local rack while picking a coordinator. Similar toDCAwareRoundRobinPolicy
but with extra option.Tests are added.
Fixes: #325