Issue 5536080: Added a sample that demonstrates email migration using multithreading
| Created: 13 years, 9 months ago by gunjansharma Modified: 13 years, 8 months ago CC: gdata-python-client-library-contributors_googlegroups.com Visibility: Public. | DescriptionAdded a test file for the sample. Patch Set 1 # Total comments: 45 Patch Set 2 : patch 1of comments resolved #Patch Set 3 : patch 1 #Patch Set 4 : patch 2 #
MessagesTotal messages: 8
Hi Gunjan, Please add tests for pretty much this entire module. Specifically, pay attention to tests for things like race conditions. How would you make this module more testable? Thanks, Vic http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... File samples/apps/email_migration_multithreading.py (right): http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:17: """Demonstrates migration of a user's mails using the Email Migration API. s/mails/email http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:20: if any of the thread receives HTTP 503 we will make of the threads receives an HTTP s/we will make/, http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:42: MESSAGE = ('Received: by 10.143.160.15 with HTTP; Mon, 16 Jul 2007' Use """ to create this string http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:44: 'Message-ID: <_message_id_@mail.gmail.com>\r\n' Use Python string templates. http://docs.python.org/library/string.html#template-strings http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:57: # Total number of messages to migrate You can remove the need for this comment by just renaming the constant to NUM_MESSAGES_TO_MIGRATE http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:59: RESET = 'operation_reset' This constant and the one below need comments. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:64: """Exception class for the Sample to show appropriate error message.""" You don't need to say "Exception class", that is already apparent from the class heading. Why does this class need an init method? http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:76: """Sample demonstrating how to migrate mails for a domain user.""" s/mails/email s/domain/Google Apps http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:119: print 'Username length should be less than 64' s/should/must http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:121: pattern = re.compile('[^\w\.\+-_\']+') http://docs.python.org/library/re.html#re.search http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:122: return not bool(pattern.search(username)) http://docs.python.org/library/re.html#re.MatchObject http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:125: """Takes a valid username as input. Gathers a valid username from STDIN. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:132: username = raw_input('Enter a valid username: ') Enter a username http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:134: print 'Invalid username' Describe why it might be invalid (>= 64 chars, etc) http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:139: """Generates a random RFC822 mail based on the message template. Generates an RFC822 email based on the message template. The generated email will have a random message ID. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:146: message = MESSAGE.replace('_message_id_', message_id) http://docs.python.org/library/string.html#template-strings http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:158: print 'I am ' + threading.currentThread().getName() print 'I am %s' % threading.currentThread().getName() http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:165: if e.message['status'] == 503 and self.retries < MAX_RETRIES: e.message.get(), in case key of 'status' does not exist. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:181: time.sleep(2 ^ tries + random.randint(0, 10)) The second try could wait 12 seconds, and in some cases this sleep would be twice as large as needed. http://code.google.com/apis/documents/docs/3.0/developers_guide_protocol.html... http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:197: """The entry point for all the threads.""" Give a better comment, this is more than an entry point. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:227: print 'Stopping %s' % thread.getName() Use STDERR for these messages or a logger, they're not output relevant to someone running the sample. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:234: """Prints the correct call for running the sample.""" Prints a message describing how to use the sample. Sign in to reply to this message.
Hello Vic Where do you want me to put the tests i.e. in the sample file itself or as a separate file? If as a separate file where do you want me to put that file? I understand it should be put in tests folder but how will I reference my sample class than? Thanks Gunjan http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... File samples/apps/email_migration_multithreading.py (right): http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:17: """Demonstrates migration of a user's mails using the Email Migration API. On 2012/01/20 18:36:09, Vic Fryzel wrote: > s/mails/email Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:17: """Demonstrates migration of a user's mails using the Email Migration API. On 2012/01/20 18:36:09, Vic Fryzel wrote: > s/mails/email Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:20: if any of the thread receives HTTP 503 we will make On 2012/01/20 18:36:09, Vic Fryzel wrote: > of the threads > receives an HTTP > s/we will make/, Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:42: MESSAGE = ('Received: by 10.143.160.15 with HTTP; Mon, 16 Jul 2007' On 2012/01/20 18:36:09, Vic Fryzel wrote: > Use """ to create this string Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:44: 'Message-ID: <_message_id_@mail.gmail.com>\r\n' On 2012/01/20 18:36:09, Vic Fryzel wrote: > Use Python string templates. > > http://docs.python.org/library/string.html#template-strings Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:57: # Total number of messages to migrate On 2012/01/20 18:36:09, Vic Fryzel wrote: > You can remove the need for this comment by just renaming the constant to > NUM_MESSAGES_TO_MIGRATE Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:59: RESET = 'operation_reset' On 2012/01/20 18:36:09, Vic Fryzel wrote: > This constant and the one below need comments. Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:64: """Exception class for the Sample to show appropriate error message.""" On 2012/01/20 18:36:09, Vic Fryzel wrote: > You don't need to say "Exception class", that is already apparent from the class > heading. > > Why does this class need an init method? http://codereview.appspot.com/5500078/diff/7003/samples/apps/emailsettings_po... http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:76: """Sample demonstrating how to migrate mails for a domain user.""" On 2012/01/20 18:36:09, Vic Fryzel wrote: > s/mails/email > s/domain/Google Apps Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:119: print 'Username length should be less than 64' On 2012/01/20 18:36:09, Vic Fryzel wrote: > s/should/must Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:121: pattern = re.compile('[^\w\.\+-_\']+') On 2012/01/20 18:36:09, Vic Fryzel wrote: > http://docs.python.org/library/re.html#re.search Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:122: return not bool(pattern.search(username)) On 2012/01/20 18:36:09, Vic Fryzel wrote: > http://docs.python.org/library/re.html#re.MatchObject Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:125: """Takes a valid username as input. On 2012/01/20 18:36:09, Vic Fryzel wrote: > Gathers a valid username from STDIN. Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:132: username = raw_input('Enter a valid username: ') On 2012/01/20 18:36:09, Vic Fryzel wrote: > Enter a username Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:134: print 'Invalid username' On 2012/01/20 18:36:09, Vic Fryzel wrote: > Describe why it might be invalid (>= 64 chars, etc) If its greater than 64 chars another message is printed by the function above. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:139: """Generates a random RFC822 mail based on the message template. On 2012/01/20 18:36:09, Vic Fryzel wrote: > Generates an RFC822 email based on the message template. > > The generated email will have a random message ID. Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:146: message = MESSAGE.replace('_message_id_', message_id) On 2012/01/20 18:36:09, Vic Fryzel wrote: > http://docs.python.org/library/string.html#template-strings Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:158: print 'I am ' + threading.currentThread().getName() On 2012/01/20 18:36:09, Vic Fryzel wrote: > print 'I am %s' % threading.currentThread().getName() Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:165: if e.message['status'] == 503 and self.retries < MAX_RETRIES: On 2012/01/20 18:36:09, Vic Fryzel wrote: > e.message.get(), in case key of 'status' does not exist. Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:181: time.sleep(2 ^ tries + random.randint(0, 10)) On 2012/01/20 18:36:09, Vic Fryzel wrote: > The second try could wait 12 seconds, and in some cases this sleep would be > twice as large as needed. > > http://code.google.com/apis/documents/docs/3.0/developers_guide_protocol.html... Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:197: """The entry point for all the threads.""" On 2012/01/20 18:36:09, Vic Fryzel wrote: > Give a better comment, this is more than an entry point. Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:227: print 'Stopping %s' % thread.getName() On 2012/01/20 18:36:09, Vic Fryzel wrote: > Use STDERR for these messages or a logger, they're not output relevant to > someone running the sample. Done. http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... samples/apps/email_migration_multithreading.py:234: """Prints the correct call for running the sample.""" On 2012/01/20 18:36:09, Vic Fryzel wrote: > Prints a message describing how to use the sample. Done. Sign in to reply to this message.
On 2012/01/21 11:14:56, gunjansharma wrote: > Hello Vic > > Where do you want me to put the tests i.e. in the sample file itself or as a > separate file? > If as a separate file where do you want me to put that file? > I understand it should be put in tests folder but how will I reference my sample > class than? It could be put in a tests package. Where are the tests packages in this project? What would be wrong with putting it in email_migration_multithreading_test.py in this same directory? > > Thanks > Gunjan > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > File samples/apps/email_migration_multithreading.py (right): > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:17: """Demonstrates migration of > a user's mails using the Email Migration API. > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > s/mails/email > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:17: """Demonstrates migration of > a user's mails using the Email Migration API. > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > s/mails/email > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:20: if any of the thread receives > HTTP 503 we will make > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > of the threads > > receives an HTTP > > s/we will make/, > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:42: MESSAGE = ('Received: by > 10.143.160.15 with HTTP; Mon, 16 Jul 2007' > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > Use """ to create this string > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:44: 'Message-ID: > <_message_id_@mail.gmail.com>\r\n' > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > Use Python string templates. > > > > http://docs.python.org/library/string.html#template-strings > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:57: # Total number of messages to > migrate > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > You can remove the need for this comment by just renaming the constant to > > NUM_MESSAGES_TO_MIGRATE > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:59: RESET = 'operation_reset' > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > This constant and the one below need comments. > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:64: """Exception class for the > Sample to show appropriate error message.""" > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > You don't need to say "Exception class", that is already apparent from the > class > > heading. > > > > Why does this class need an init method? > http://codereview.appspot.com/5500078/diff/7003/samples/apps/emailsettings_po... > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:76: """Sample demonstrating how > to migrate mails for a domain user.""" > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > s/mails/email > > s/domain/Google Apps > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:119: print 'Username length > should be less than 64' > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > s/should/must > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:121: pattern = > re.compile('[^\w\.\+-_\']+') > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > http://docs.python.org/library/re.html#re.search > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:122: return not > bool(pattern.search(username)) > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > http://docs.python.org/library/re.html#re.MatchObject > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:125: """Takes a valid username as > input. > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > Gathers a valid username from STDIN. > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:132: username = raw_input('Enter > a valid username: ') > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > Enter a username > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:134: print 'Invalid username' > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > Describe why it might be invalid (>= 64 chars, etc) > > If its greater than 64 chars another message is printed by the function above. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:139: """Generates a random RFC822 > mail based on the message template. > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > Generates an RFC822 email based on the message template. > > > > The generated email will have a random message ID. > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:146: message = > MESSAGE.replace('_message_id_', message_id) > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > http://docs.python.org/library/string.html#template-strings > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:158: print 'I am ' + > threading.currentThread().getName() > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > print 'I am %s' % threading.currentThread().getName() > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:165: if e.message['status'] == > 503 and self.retries < MAX_RETRIES: > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > e.message.get(), in case key of 'status' does not exist. > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:181: time.sleep(2 ^ tries + > random.randint(0, 10)) > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > The second try could wait 12 seconds, and in some cases this sleep would be > > twice as large as needed. > > > > > http://code.google.com/apis/documents/docs/3.0/developers_guide_protocol.html... > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:197: """The entry point for all > the threads.""" > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > Give a better comment, this is more than an entry point. > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:227: print 'Stopping %s' % > thread.getName() > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > Use STDERR for these messages or a logger, they're not output relevant to > > someone running the sample. > > Done. > > http://codereview.appspot.com/5536080/diff/1/samples/apps/email_migration_mul... > samples/apps/email_migration_multithreading.py:234: """Prints the correct call > for running the sample.""" > On 2012/01/20 18:36:09, Vic Fryzel wrote: > > Prints a message describing how to use the sample. > > Done. Sign in to reply to this message.
Gentle Reminder Thanks Gunjan Sharma | Developer Programs Engineer | gunjansharma@google.com | +91 7702534446 On Mon, Jan 23, 2012 at 10:01 PM, <gunjansharma@google.com> wrote: > http://codereview.appspot.com/**5536080/<http://codereview.appspot.com/5536080/> > Sign in to reply to this message. | |||||||||||||||||||||||||||||||||||||||||||
