Opened 6 years ago

Closed 5 years ago

#6705 enhancement closed fixed (fixed)

Stop _AttemptManager from using Queue.noisy

Reported by: Stacey Sern Owned by: hawkowl
Priority: normal Milestone:
Component: mail Keywords:
Cc: Branch: branches/attemptmanager-noisy-6705-2
branch-diff, diff-cov, branch-cov, buildbot
Author: shira

Description

Ticket #4623 points out that relaymanager._AttemptManager directly uses relaymanager.Queue.noisy. So as not to include .noisy in the new IRelayQueue interface, _AttemptManager should not use that attribute.

Change History (20)

comment:1 Changed 6 years ago by Stacey Sern

Author: shira
Branch: branches/attemptmanager-noisy-6705

(In [39782]) Branching to 'attemptmanager-noisy-6705'

comment:2 Changed 6 years ago by Stacey Sern

(In [39981]) Initial commit

refs #6705

comment:3 Changed 6 years ago by Stacey Sern

(In [39982]) Fix twistedchecker errors

refs #6705

comment:4 Changed 6 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

comment:5 Changed 6 years ago by hawkowl

Owner: set to hawkowl

comment:6 Changed 6 years ago by hawkowl

Keywords: review removed
Owner: changed from hawkowl to Stacey Sern

Hi shira, thanks for working on this!

  1. (Line 2229, test_mail.py) "realyed" should be "relayed"
  1. The epytext for Queue should be defined in its init function, not in the class. (Line 2242, test_mail.py)
  1. Fullstops required on line 2261, 2243 in test_mail.py.

Minor things which I think might be worth fixing while you're touching code nearby, but is your call:

  1. There is incorrect indentation on line 325.
  1. Lines 314 and 315 could probably be pulled together. I'm also not sure that it actually does as it says? Maybe I'm just not understanding the code.
  1. Line 320 is too long.
  1. _AttemptManager has incorrect spacing above it.

Please fix/address these, and the minor things if you think the scope of this ticket is appropriate, and resubmit. FWIW, I can't seem to find anything else wrong, other than those two small typo points, and the builds are all green.

PS: Unrelated to the review, line 2363 of test_mail.py (self.assertTrue(am.noisy)) did make me giggle :)

comment:7 Changed 6 years ago by Stacey Sern

(In [40051]) Incorporate review comments

refs #6705

comment:8 Changed 6 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

I fixed major items 1-3. I also fixed minor items 1 and 3. Item 4 will be fixed as part of ticket #6739.

Regarding minor item #2, I agree that what the comment says is not implemented. Ticket #6726 documents a problem related to that. I did not change the comment since it should be addressed by #6726.

comment:9 Changed 6 years ago by hawkowl

Hi shira, thanks! I'll leave someone else to review it now, just in case I've overlooked anything.

RE: #6739, feel free to assign it to me when you put it up for review, and I'll take a look.

comment:10 Changed 6 years ago by lvh

Owner: set to lvh

comment:11 Changed 6 years ago by lvh

Keywords: review removed
Owner: changed from lvh to Stacey Sern

Hi shira,

Thanks for working on this some more :) This is one of those great tickets where the state of the code after the merge is so much better than the state before :)

Small notes (not really review points):

  • If the reactor isn't passed, the code imports the global reactor as globalReactor, and then assigns reactor to it. globalReactor doesn't appear to be used anywhere else. Is that intentional? From what I can tell just from twisted.internet import reactor would do the same thing.
  • We may at some point want to use #6365 for this instead of yet another manual deferred notification thing, but it seems that won't be closed before this is.
  • I'm unfamiliar with mail, but in the docstring for def notifySuccess(self, relay, message): it may be good to clarify that you're removing it from the queue (that clarified it for me at least)
  • I'm not a huge fan of the from_ and to variable names (how about sender, receiver?) but I think they're used all over t.mail already so maybe the damage done from inconsistency is greater than any naming improvement :)
  • Is the hardcoded 30 second wait time in notifyNoConnection intentional?
  • The "Set up" wording of the notify* calls is kind of confusing to me, I had to re-read the docstring a few times before I grokked it. Perhaps "prepare"?
  • There doesn't appear to be a difference between "done" as in notifyDone and "completed" as in getCompletionDeferred. Is that intentional?

I did find a bug in one of the docstrings:

  • notifyDone claims to fire a Deferred, but it actually fires *all* of the notification deferreds. Perhaps a reference to getCompletionDeferred?

Other than that, looks great to me as well as the buildbots. I don't know if any of my nitpicks hold any water, I'm more than happy to merge this as-is modulo the minor docstring bug :)

cheers

lvh

comment:12 Changed 5 years ago by Stacey Sern

Branch: branches/attemptmanager-noisy-6705branches/attemptmanager-noisy-6705-2

(In [43028]) Branching to 'attemptmanager-noisy-6705-2'

comment:13 Changed 5 years ago by Stacey Sern

(In [43030]) Remove conflict refs #6705

comment:14 Changed 5 years ago by Stacey Sern

(In [43031]) Address review comments

refs #6705

comment:15 Changed 5 years ago by Stacey Sern

(In [43033]) Remove trailing whitespace

refs #6705

comment:16 Changed 5 years ago by Stacey Sern

(In [43034]) Improve docstring

refs #6705

comment:17 Changed 5 years ago by Stacey Sern

Keywords: review added
Owner: changed from Stacey Sern to Glyph

I addressed most of the issues in comment:11. I do not know if either the hard-coded 30 seconds or the difference between done and complete are intentional. Regarding the 30 seconds, I can't figure out but would like to understand why the calls to setWaiting are delayed.

comment:18 Changed 5 years ago by Glyph

Owner: Glyph deleted

Un-assigning all review tickets assigned to me. Unless there's a specific reason I need to review something, let's leave them unassigned :-).

comment:19 Changed 5 years ago by hawkowl

Keywords: review removed
Owner: set to hawkowl

This looks good to me except for a minor whitespace thing which I'll fix.

Will merge. Thanks!

comment:20 Changed 5 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [43683]) Merge attemptmanager-noisy-6705-2: Stop _AttemptManager from using Queue.noisy

Author: shira Reviewers: hawkowl, lvh Fixes: #6705

Note: See TracTickets for help on using tickets.