-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
The first patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=694444 by not detecting duplicates at all.
The second patch is an optimization. Since it is possible that the list of servers contains duplicates now, the code marks all duplicates when setting a server status, too. I'm submitting it as a separate patch because it might be easier to push only one of them if QE thinks they have the capacity to test only one of them for the stable 1.5 branch.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/11/2011 07:01 AM, Jakub Hrozek wrote:
The first patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=694444 by not detecting duplicates at all.
Ack.
The second patch is an optimization. Since it is possible that the list of servers contains duplicates now, the code marks all duplicates when setting a server status, too. I'm submitting it as a separate patch because it might be easier to push only one of them if QE thinks they have the capacity to test only one of them for the stable 1.5 branch.
Nack.
While it's not as if we call this function often, it seems counter-intuitive to me that we'd be looping through all the server lists (which could be arbitrarily long, if we're using SRV records) on every state update, rather than just eating this cost once when adding new servers to the list.
In other words, we should do a duplicate check before calling create_fo_server() and skip creating it if the name and port matches.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/11/2011 12:07 PM, Stephen Gallagher wrote:
On 04/11/2011 07:01 AM, Jakub Hrozek wrote:
The first patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=694444 by not detecting duplicates at all.
Ack.
Pushed the first patch to master and sssd-1-5.
The second patch is an optimization. Since it is possible that the list of servers contains duplicates now, the code marks all duplicates when setting a server status, too. I'm submitting it as a separate patch because it might be easier to push only one of them if QE thinks they have the capacity to test only one of them for the stable 1.5 branch.
Nack.
While it's not as if we call this function often, it seems counter-intuitive to me that we'd be looping through all the server lists (which could be arbitrarily long, if we're using SRV records) on every state update, rather than just eating this cost once when adding new servers to the list.
In other words, we should do a duplicate check before calling create_fo_server() and skip creating it if the name and port matches.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On 04/11/2011 06:07 PM, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/11/2011 07:01 AM, Jakub Hrozek wrote:
The first patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=694444 by not detecting duplicates at all.
Ack.
The second patch is an optimization. Since it is possible that the list of servers contains duplicates now, the code marks all duplicates when setting a server status, too. I'm submitting it as a separate patch because it might be easier to push only one of them if QE thinks they have the capacity to test only one of them for the stable 1.5 branch.
Nack.
While it's not as if we call this function often, it seems counter-intuitive to me that we'd be looping through all the server lists (which could be arbitrarily long, if we're using SRV records) on every state update, rather than just eating this cost once when adding new servers to the list.
In other words, we should do a duplicate check before calling create_fo_server() and skip creating it if the name and port matches.
I actually think it is more convenient to allow duplicates than to detect them on create_fo_server() time and disallow them. Let me explain:
It is quite possible (and actually the default for ipa-client-install) to have the same server name as a result of a SRV query and a hard coded hostname. The problem is the ordering of servers.
Consider that srv query resolves to "ldap.example.com, ldap2.example.com" and the config file contains "_srv_, ldap.example.com". If we discard duplicates after the SRV query is resolved, the order would be "ldap2.example.com, ldap.example.com" which is wrong.
We could reorder the list and then, when the SRV query expires, reorder it back again, but I suspect that it is quite a bit more work and I would prefer this to be tracked as a separate issue in 1.6 or 1.7.
Jakub
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/11/2011 04:36 PM, Jakub Hrozek wrote:
On 04/11/2011 06:07 PM, Stephen Gallagher wrote:
Nack.
While it's not as if we call this function often, it seems counter-intuitive to me that we'd be looping through all the server lists (which could be arbitrarily long, if we're using SRV records) on every state update, rather than just eating this cost once when adding new servers to the list.
In other words, we should do a duplicate check before calling create_fo_server() and skip creating it if the name and port matches.
I actually think it is more convenient to allow duplicates than to detect them on create_fo_server() time and disallow them. Let me explain:
It is quite possible (and actually the default for ipa-client-install) to have the same server name as a result of a SRV query and a hard coded hostname. The problem is the ordering of servers.
Consider that srv query resolves to "ldap.example.com, ldap2.example.com" and the config file contains "_srv_, ldap.example.com". If we discard duplicates after the SRV query is resolved, the order would be "ldap2.example.com, ldap.example.com" which is wrong.
Why would that be the order? Wouldn't it be simplest to just check the list before adding each entry, and if it already exists, just not add it?
The result then would be ldap.example.com, ldap2.example.com (because the attempt to add the hard-coded name after the _SRV_ processing would be ignored since it's already present (and appropriately ordered).
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/14/2011 07:54 PM, Stephen Gallagher wrote:
On 04/11/2011 04:36 PM, Jakub Hrozek wrote:
On 04/11/2011 06:07 PM, Stephen Gallagher wrote:
Nack.
While it's not as if we call this function often, it seems counter-intuitive to me that we'd be looping through all the server lists (which could be arbitrarily long, if we're using SRV records) on every state update, rather than just eating this cost once when adding new servers to the list.
In other words, we should do a duplicate check before calling create_fo_server() and skip creating it if the name and port matches.
I actually think it is more convenient to allow duplicates than to detect them on create_fo_server() time and disallow them. Let me explain:
It is quite possible (and actually the default for ipa-client-install) to have the same server name as a result of a SRV query and a hard coded hostname. The problem is the ordering of servers.
Consider that srv query resolves to "ldap.example.com, ldap2.example.com" and the config file contains "_srv_, ldap.example.com". If we discard duplicates after the SRV query is resolved, the order would be "ldap2.example.com, ldap.example.com" which is wrong.
Why would that be the order? Wouldn't it be simplest to just check the list before adding each entry, and if it already exists, just not add it?
The result then would be ldap.example.com, ldap2.example.com (because the attempt to add the hard-coded name after the _SRV_ processing would be ignored since it's already present (and appropriately ordered).
Right, the problem is that this is done dynamically and the SRV lookups can expire, which means that if you skip adding a hardcoded name, you need to add it back at the right place when the SRV query expires.
It's not impossible, it just seemed like a lot of work for a little gain - -- and actually if we gown down this route, I would prefer skipping the duplicate that was resolved from SRV query and place the hardcoded "original" there.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/15/2011 05:08 AM, Jakub Hrozek wrote:
On 04/14/2011 07:54 PM, Stephen Gallagher wrote:
On 04/11/2011 04:36 PM, Jakub Hrozek wrote:
On 04/11/2011 06:07 PM, Stephen Gallagher wrote:
Nack.
While it's not as if we call this function often, it seems counter-intuitive to me that we'd be looping through all the server lists (which could be arbitrarily long, if we're using SRV records) on every state update, rather than just eating this cost once when adding new servers to the list.
In other words, we should do a duplicate check before calling create_fo_server() and skip creating it if the name and port matches.
I actually think it is more convenient to allow duplicates than to detect them on create_fo_server() time and disallow them. Let me explain:
It is quite possible (and actually the default for ipa-client-install) to have the same server name as a result of a SRV query and a hard coded hostname. The problem is the ordering of servers.
Consider that srv query resolves to "ldap.example.com, ldap2.example.com" and the config file contains "_srv_, ldap.example.com". If we discard duplicates after the SRV query is resolved, the order would be "ldap2.example.com, ldap.example.com" which is wrong.
Why would that be the order? Wouldn't it be simplest to just check the list before adding each entry, and if it already exists, just not add it?
The result then would be ldap.example.com, ldap2.example.com (because the attempt to add the hard-coded name after the _SRV_ processing would be ignored since it's already present (and appropriately ordered).
Right, the problem is that this is done dynamically and the SRV lookups can expire, which means that if you skip adding a hardcoded name, you need to add it back at the right place when the SRV query expires.
It's not impossible, it just seemed like a lot of work for a little gain -- and actually if we gown down this route, I would prefer skipping the duplicate that was resolved from SRV query and place the hardcoded "original" there.
You've convinced me. I think your original patch is probably the most sensible. Ack.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/15/2011 08:42 AM, Stephen Gallagher wrote:
On 04/15/2011 05:08 AM, Jakub Hrozek wrote:
On 04/14/2011 07:54 PM, Stephen Gallagher wrote:
On 04/11/2011 04:36 PM, Jakub Hrozek wrote:
On 04/11/2011 06:07 PM, Stephen Gallagher wrote:
Nack.
While it's not as if we call this function often, it seems counter-intuitive to me that we'd be looping through all the server lists (which could be arbitrarily long, if we're using SRV records) on every state update, rather than just eating this cost once when adding new servers to the list.
In other words, we should do a duplicate check before calling create_fo_server() and skip creating it if the name and port matches.
I actually think it is more convenient to allow duplicates than to detect them on create_fo_server() time and disallow them. Let me explain:
It is quite possible (and actually the default for ipa-client-install) to have the same server name as a result of a SRV query and a hard coded hostname. The problem is the ordering of servers.
Consider that srv query resolves to "ldap.example.com, ldap2.example.com" and the config file contains "_srv_, ldap.example.com". If we discard duplicates after the SRV query is resolved, the order would be "ldap2.example.com, ldap.example.com" which is wrong.
Why would that be the order? Wouldn't it be simplest to just check the list before adding each entry, and if it already exists, just not add it?
The result then would be ldap.example.com, ldap2.example.com (because the attempt to add the hard-coded name after the _SRV_ processing would be ignored since it's already present (and appropriately ordered).
Right, the problem is that this is done dynamically and the SRV lookups can expire, which means that if you skip adding a hardcoded name, you need to add it back at the right place when the SRV query expires.
It's not impossible, it just seemed like a lot of work for a little gain -- and actually if we gown down this route, I would prefer skipping the duplicate that was resolved from SRV query and place the hardcoded "original" there.
You've convinced me. I think your original patch is probably the most sensible. Ack.
Patch 0002 pushed to master.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Stephen Gallagher