-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
Hi all,
After some discussion with Matt on my pull request to make mirrorlist use threads instead of forks, I found one issue with the current code that breaks the ASN-based lookup: it changes the in-memory version of the ASN lookup tree.
Could I get +1s to apply the underneath patch, which will fix this issue by making a thread-local copy of the tree before searching and modifying?
commit 8a9d389bc77fe49086458df7ca03f9192a711e79 Author: Patrick Uiterwijk puiterwijk@redhat.com Date: Tue Oct 20 01:39:58 2015 +0200
mirrorlist: Use a thread-local copy of the tree to prevent changing the global one
Signed-off-by: Patrick Uiterwijk puiterwijk@redhat.com
diff --git a/files/hotfix/mirrorlist/mirrorlist_server.py b/files/hotfix/mirrorlist/mirrorlist_server.py index 39dfc7a..b884229 100755 - --- a/files/hotfix/mirrorlist/mirrorlist_server.py +++ b/files/hotfix/mirrorlist/mirrorlist_server.py @@ -6,6 +6,7 @@
# standard library modules in alphabetical order from collections import defaultdict +import copy import datetime import getopt import logging @@ -234,11 +235,12 @@ def tree_lookup(tree, ip, field, maxResults=None): # and we'll get a new copy of the tree from our parent the next time it # fork()s. # returns a list of tuples (prefix, data) + ltree = copy.deepcopy(tree) result = [] len_data = 0 if ip is None: return result - - node = tree.search_best(ip.strNormal()) + node = ltree.search_best(ip.strNormal()) while node is not None: prefix = node.prefix if type(node.data[field]) == list: @@ -248,8 +250,8 @@ def tree_lookup(tree, ip, field, maxResults=None): t = (prefix, node.data[field],) result.append(t) if maxResults is None or len_data < maxResults: - - tree.delete(prefix) - - node = tree.search_best(ip.strNormal()) + ltree.delete(prefix) + node = ltree.search_best(ip.strNormal()) else: break return result
- -- With kind regards, Patrick Uiterwijk Fedora Infra
Looks ok. Would it be ok to put comments in the future about what the change does since it doesn't seem to match exactly what the above comment says? [Or did I misread that ocmment?]
On 19 October 2015 at 17:41, Patrick Uiterwijk puiterwijk@redhat.com wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
Hi all,
After some discussion with Matt on my pull request to make mirrorlist use threads instead of forks, I found one issue with the current code that breaks the ASN-based lookup: it changes the in-memory version of the ASN lookup tree.
Could I get +1s to apply the underneath patch, which will fix this issue by making a thread-local copy of the tree before searching and modifying?
commit 8a9d389bc77fe49086458df7ca03f9192a711e79 Author: Patrick Uiterwijk puiterwijk@redhat.com Date: Tue Oct 20 01:39:58 2015 +0200
mirrorlist: Use a thread-local copy of the tree to prevent changing the global one Signed-off-by: Patrick Uiterwijk <puiterwijk@redhat.com>diff --git a/files/hotfix/mirrorlist/mirrorlist_server.py b/files/hotfix/mirrorlist/mirrorlist_server.py index 39dfc7a..b884229 100755
- --- a/files/hotfix/mirrorlist/mirrorlist_server.py
+++ b/files/hotfix/mirrorlist/mirrorlist_server.py @@ -6,6 +6,7 @@
# standard library modules in alphabetical order from collections import defaultdict +import copy import datetime import getopt import logging @@ -234,11 +235,12 @@ def tree_lookup(tree, ip, field, maxResults=None): # and we'll get a new copy of the tree from our parent the next time it # fork()s. # returns a list of tuples (prefix, data)
- ltree = copy.deepcopy(tree) result = [] len_data = 0 if ip is None: return result
- node = tree.search_best(ip.strNormal())
- node = ltree.search_best(ip.strNormal()) while node is not None: prefix = node.prefix if type(node.data[field]) == list:
@@ -248,8 +250,8 @@ def tree_lookup(tree, ip, field, maxResults=None): t = (prefix, node.data[field],) result.append(t) if maxResults is None or len_data < maxResults:
tree.delete(prefix)
node = tree.search_best(ip.strNormal())
ltree.delete(prefix)node = ltree.search_best(ip.strNormal()) else: break
With kind regards, Patrick Uiterwijk Fedora Infra -----BEGIN PGP SIGNATURE----- Version: GnuPG v1
iQIcBAEBCgAGBQJWJX+bAAoJEIZXmA2atR5Q/yEP/2A9Vy9H2e4u1aoKNZnwaKNB AQagkPfQ7CgaVvutu7OHWpyP5G+UFWtTQEQlY8rRVSAU4NKYBEABw4Rk+dKt+iF5 WO9VGeqVcvPqV1cPGBqW+qIYJ/ZzdEhg+jEA3v6LInUwukUybCxnnb5szyWgARQm 91mXovdzanTovqCUhlHpDu6fxD/mkLl9QOsElHvZZjJNNt5mTy0+utnCM9Db7O2i CmIzfAZp5WG6YtottNZ5KoGPCgr/RmtKtNRyfYvQ9yC6T0S8D33sNUShmPZtQ4CH O8c0T11xd0JEkQSMQ5pGQzTMTYbbOkahdpx20jlpQDVij3PZbUw31cMG/0SF8mQf Hf1nbruMl1VpMV77dioyycTskR88A7voc0dxyP7lw5Io/ou/o1+qx8u9LYz4OelK Sik5Jy5Vc8NjxvjLGyskGThAmJnfck9xnl6odJZhQpaGjHwhKFrN7L70upQ4rvsH MwGIy7ycRmXZrv4gl8ubYY/FKmHhftOcWMyAI32jU2ES43Q2DIo1T0Fs2bRZZsIf iGnfKTDh61xU++9R9D+BvUk6vJvBle14TqisVOv9T0T/Uu8tneEkvcmEiaCW8fi2 f9teDc8Qwe3t6Dr1K858HMBosriCy8E2OBe08555XBIH+pF8rAVrx049b4Wpxfzf zY9cqa5tvAKAkRIvM5Ks =m5h0 -----END PGP SIGNATURE----- _______________________________________________ infrastructure mailing list infrastructure@lists.fedoraproject.org http://lists.fedoraproject.org/admin/infrastructure@lists.fedoraproject.org
Looks ok. Would it be ok to put comments in the future about what the change does since it doesn't seem to match exactly what the above comment says? [Or did I misread that ocmment?]
commit 8a9d389bc77fe49086458df7ca03f9192a711e79 Author: Patrick Uiterwijk puiterwijk@redhat.com Date: Tue Oct 20 01:39:58 2015 +0200
mirrorlist: Use a thread-local copy of the tree to prevent changing the global one Signed-off-by: Patrick Uiterwijk <puiterwijk@redhat.com>
Well, the commit message is a deeper explanation of what's going on here, but I will add the short summary in my original email to the commit message as well before pushing.
-- Stephen J Smoogen. _______________________________________________ infrastructure mailing list infrastructure@lists.fedoraproject.org http://lists.fedoraproject.org/admin/infrastructure@lists.fedoraproject.org
So, I deployed this to some servers first to test, but they were again falling under the load (since they were having to do a lot of deep copies, and that took more CPU time than I had expected). As such, I reverted this patch, and will be looking at a better way to fix this.
With kind regards, Patrick Uiterwijk Fedora Infra
----- Original Message -----
+1 here
kevin
infrastructure mailing list infrastructure@lists.fedoraproject.org http://lists.fedoraproject.org/admin/infrastructure@lists.fedoraproject.org
infrastructure@lists.fedoraproject.org
-
Kevin Fenzi -
Patrick Uiterwijk -
Stephen John Smoogen