Several code enhancements to git_add_course #1985
Conversation
|
@nedbat |
|
I just did a lot of testing on this in a production multi threaded environment, and it appears that the logging handlers really are thread safe. I was importing several courses at the same time, and though the system log was interleaved with messages from both imports, the actual captured log did not have any bleed through. |
|
I haven't reviewed the code yet, but I am curious how it can be that the logs aren't interleaved? Are you certain you're running multi-threaded? Is there some threading protection in the log that I don't know about? |
|
I should say it is using gunicorn with workers. I am certain the logs aren't interleaved, but that technically isn't multithreaded, it is forked so that is probably what explains the unexpected behavior. I also noticed when digging that the loggers being used aren't stock python loggers, they are celery loggers so I don't knwo what differences there may be there. |
|
Can this PR be split in two? One just to fix the noise and stray files during testing, and another for the other changes? We'd like to fix the testing things soon. |
|
They are fairly tightly integrated since one of the three changes was to switch from return codes to exception messages which changed the testing structure from assertEquals to raises. The only other two changes were to move the main function to a new file and remove the monkey patch, so there isn't too much going on here. |
There was a problem hiding this comment.
There's no need to translate log.debug messages, or you need to translate all of them. Do these appear somewhere other than the console?
There was a problem hiding this comment.
They get captured into the mongo log, and output to the screen on import. Is there somewhere I'm missing a translate? I could pull them all, but I think most of them already are.
There was a problem hiding this comment.
Oh, maybe I saw log.debug("location = {0}"), which isn't translated, but doesn't need to be.
I'm working on i18n now, and I'm not sure it's good to add all these strings to the translators' work load. Do we have expectations that other sites will use this feature and need it presented in another language?
There was a problem hiding this comment.
I can pull that all out of the logs if that will make things easier. I don't know if other sites will want it honestly. It is a handy feature and from the code list I would guess there are at least a few people that are editing courseware in xml and may want some of this convenience, but I'll leave it up to you. I pushed changes to address your comments.
There was a problem hiding this comment.
If you're OK with it, I would prefer not to have these strings localized. We aren't even attempting to finish i18n for the instructor dashboard as a whole. We can come back to do it later if people ask about it.
Sorry for the churn... :(
|
My only concern now is about the translation. Do these strings need to be translated? |
|
I removed a lot of the gettext phrases as requested. I still left much of it in the regular sysadmin dashboard interface since I think that might be helpful for other openedx users that may not speak English. |
|
Great, once the tests pass, merge it! 👍 |
|
Looks like Jenkins messed up, should I just try and rebase and push again? I haven't seen that particular error from it. |
|
I'll start a build manually. |
Improved testing (cleaning up afterwards, catching stdout) Refactored import function to it's own file Removed monkey patch to log capturing, but still needs work Translation fixes Removal of several gettext phrases
Several code enhancements to git_add_course
Improved testing (cleaning up afterwards, catching stdout)
Refactored import function to it's own file
Removed monkey patch to log capturing, but still needs work