URL: https://github.com/SSSD/sssd/pull/420 Author: mzidek-rh Title: #420: NSS: memcache_timeout=0 Action: opened
PR body: """ This is PR is continuation of: https://github.com/SSSD/sssd/pull/416
about new semantics of memcache_timeout=0 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/420/head:pr420 git checkout pr420
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
lslebodn commented: """ Please keep change minimal. As it was agreen on team meeting """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338184963
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
mzidek-rh commented: """ I really do believe that it is better to move the memcache related code from nss_process_init to a separate function and I think it is better to do moves of this kind before downstream rebases.
There are several reason as to why I moved the code there: - there is already too much memcache related code in the nss_process_init and moving it to a static function makes things more readable - I do not like 'cutting' the nss_process_init function with one check for timeout==0 and then returning from the function, because there is some code after the memcache initialization that should be executed and also we could shoot ourselves in the future by adding non memcache related code after this return that would not be executed when the timeout is zero. So the only way to solve this is by putting the whole memcache related stuff into one big if (not good) or move it to a function. I know that there is also the possibility that you used in your PR to put the condition in the mmap_cache_init function, because IMO it makes more sense to put this to a function where we read the timeout value (or all memcache init values to be precise) because it is obvious what effects these options have to memcache from reading just this part of the code. This benefit will IMO even more obvious once we add the options to change memcache sizes.
All in all I do not think it is beneficial for us to go with the minimum lines of code this time. """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338190197
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
mzidek-rh commented: """ I really do believe that it is better to move the memcache related code from nss_process_init to a separate function and I think it is better to do moves of this kind before downstream rebases.
There are several reason as to why I moved the code there: - there is already too much memcache related code in the nss_process_init and moving it to a static function makes things more readable - I do not like 'cutting' the nss_process_init function with one check for timeout==0 and then returning from the function, because there is some code after the memcache initialization that should be executed and also we could shoot ourselves in the future by adding non memcache related code after this return that would not be executed when the timeout is zero. So the only way to solve this is by putting the whole memcache related stuff into one big if (not good) or move it to a function. I know that there is also the possibility that you used in your PR to put the condition in the mmap_cache_init function, but IMO it makes more sense to put this to a function where we read the timeout value (or all memcache init values to be precise) because it is obvious what effects these options have to memcache from reading just this part of the code. This benefit will IMO even more obvious once we add the options to change memcache sizes.
All in all I do not think it is beneficial for us to go with the minimum lines of code this time. """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338190197
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
mzidek-rh commented: """ I really do believe that it is better to move the memcache related code from nss_process_init to a separate function and I think it is better to do moves of this kind before downstream rebases.
There are several reason as to why I moved the code there: - there is already too much memcache related code in the nss_process_init and moving it to a static function makes things more readable - I do not like 'cutting' the nss_process_init function with one check for timeout==0 and then returning from the function, because there is some code after the memcache initialization that should be executed and also we could shoot ourselves in the future by adding non memcache related code after this return that would not be executed when the timeout is zero. So the only way to solve this is by putting the whole memcache related stuff into one big if (not good) or move it to a function. I know that there is also the possibility that you used in your PR to put the condition in the mmap_cache_init function, but IMO it makes more sense to put this to a function where we read the timeout value (or all memcache init values to be precise) because it is obvious what effects these options have to memcache from reading just this function. This benefit will be IMO even more obvious once we add the options to change memcache sizes.
All in all I do not think it is beneficial for us to go with the minimum lines of code this time. """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338190197
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
mzidek-rh commented: """ I really do believe that it is better to move the memcache related code from nss_process_init to a separate function and I think it is better to do moves of this kind before downstream rebases.
There are several reason as to why I moved the code there: - there is already too much memcache related code in the nss_process_init and moving it to a static function makes things more readable - I do not like 'cutting' the nss_process_init function with one check for timeout==0 and then returning from the function, because there is some code after the memcache initialization that should be executed and also we could shoot ourselves in the future by adding non memcache related code after this return that would not be executed when the timeout is zero. So the only way to solve this is by putting the whole memcache related stuff into one big 'if statement' (not good) or move it to a function. I know that there is also the possibility that you used in your PR to put the condition in the mmap_cache_init function, but IMO it makes more sense to put this to a function where we read the timeout value (or all memcache init values to be precise) because it is obvious what effects these options have to memcache from reading just this function. This benefit will be IMO even more obvious once we add the options to change memcache sizes.
All in all I do not think it is beneficial for us to go with the minimum lines of code this time. """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338190197
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
mzidek-rh commented: """ I removed the changes requested label without changing the code (explanation is in my previous comment). """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338191769
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
jhrozek commented: """ I think there might also be confusion around what the "minimal" approach as agreed on the meeting is. I don't think it was meant as trying to go for the minimal amount of lines changed, but at least I understood it as us dropping PR #390 and introducing this one. Even with the split of a block of code into a new function, the patch is not very risky.
In addition, we agreed to not introduce a configure time option if I recall correctly (and the meeting notes explicitly say "without conditionals") """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338198343
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
jhrozek commented: """ The patch seems to work OK, but there is no test..I wrote a quick one in https://github.com/jhrozek/sssd/tree/review if you think it would be enough, can you squash it into your patch and resubmit? """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338212006
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
jhrozek commented: """ Please see the note about the test and the nitpick in the man page and the debug level.. """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338212783
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
lslebodn commented: """ Minimal means minimal. And yesterday there were just two ways which were discussed https://github.com/SSSD/sssd/pull/390/files and https://github.com/SSSD/sssd/pull/390#issuecomment-334448236
"""
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338213789
URL: https://github.com/SSSD/sssd/pull/420 Author: mzidek-rh Title: #420: NSS: memcache_timeout=0 Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/420/head:pr420 git checkout pr420
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
mzidek-rh commented: """ @jhrozek I included the test you wrote. It LGTM, I am currently running the intgcheck suite locally to see if it passes. The only change I made to the test was that I added a blank line to suppress pep8 warning.
The other nitpicks are also addressed. """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338221566
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
mzidek-rh commented: """ Btw. feel free to add your Signed-of-by tag to the that I merged with your test. """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338221932
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
lslebodn commented: """ Thank you very much for changes.
To person who will push the patches. Please do not mention my name anywhere in commit messages. I do not want to have anything related with this misfeature. """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338238625
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
mzidek-rh commented: """ @lslebodn about the signed-off-by, I was talking to @jhrozek """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338240608
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
mzidek-rh commented: """ @lslebodn about the signed-off-by, I was talking to @jhrozek ... because I squashed his code into my patch """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338240608
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
lslebodn commented: """ On (20/10/17 08:31), mzidek-rh wrote:
@lslebodn about the signed-off-by, I was talking to @jhrozek
We also use Review-by, and I can even see Tested-by in git log
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338241198
URL: https://github.com/SSSD/sssd/pull/420 Title: #420: NSS: memcache_timeout=0
mzidek-rh commented: """ @lslebodn well I know that, I just thought you are reacting to that signed-off-by comment I made. I see it probably was not the case. """
See the full comment at https://github.com/SSSD/sssd/pull/420#issuecomment-338241941
sssd-devel@lists.fedorahosted.org
-
jhrozek -
lslebodn
-
mzidek-rh