Opened 7 years ago

Last modified 6 years ago

#6717 enhancement new

Improve SmartHostSMTPRelayingManager allocation of messages to relayers

Reported by: Stacey Sern Owned by: Stacey Sern
Priority: normal Milestone:
Component: mail Keywords:
Cc: Branch: branches/smarthost-msg-alloc-6717-3
branch-diff, diff-cov, branch-cov, buildbot
Author: shira

Description

SmartHostSMTPRelayingManager has two optional initialization parameters, maxConnections and maxMessagesPerConnection. maxMessagesPerConnection is not used at all. Depending on the value of maxConnections (which defaults to 2) and the number of destination domains represented in the relay queue, the number of messages assigned to a relayer for a domain may be far fewer than the number of messages for the domain in the relay queue.

SmartHostSMTPRelayingManager._checkStateMX gets a list of all waiting message from the queue. It then goes through the messages creating a dictionary of all messages for each represented domain. However, it stops as soon as the number of different domains it comes across plus the number of existing managed relayers equals maxConnections.

So if there are no relayers being managed and 15 messages in the relay queue and the first one is for domain a and the second is for domain b, _checkStateMX will stop going through the list of messages after the second one and will create a relayer for domain A to handle one message and another relayer for domain B to handle one message and 13 messages will remain waiting in the queue even if they are for domain A or domain B.

If the first 13 messages in the queue are for domain A and the 14th is for domain B, _checkStateMX will create a relayer for domain A to handle 13 messages and a relayer for domain B to handle one message and one message will remain waiting in the queue.

What should happen for a queue with messages for domain A and domainB is that all the messages in the queue for domain A, up to maxMessagesPerConnection messages, should be assigned to one relayer and up to maxMessagesPerConnection messages from the queue for domain B should be assigned to another relayer, assuming maxConnections = 2.

Change History (20)

comment:1 Changed 7 years ago by Stacey Sern

Owner: set to Stacey Sern

comment:2 Changed 7 years ago by Stacey Sern

Author: shira
Branch: branches/smarthost-msg-alloc-6717

(In [39844]) Branching to 'smarthost-msg-alloc-6717'

comment:3 Changed 7 years ago by Stacey Sern

(In [39847]) Initial commit

refs #6717

comment:4 Changed 7 years ago by Stacey Sern

(In [39848]) Fix twistedchecker and test relay queue problems

refs #6717

comment:5 Changed 7 years ago by Stacey Sern

(In [39849]) Fix test relay queue

refs #6717

comment:6 Changed 7 years ago by Stacey Sern

(In [39850]) Fix test relay queue to work on all platforms

refs #6717

comment:7 Changed 7 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

comment:8 Changed 7 years ago by lvh

Owner: set to lvh

comment:9 Changed 7 years ago by lvh

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

Thanks for working on this and many other tickets!

Buildbots look clean, just the usual spurious ENOSPC failure on win32.

Some minor nitpicks:

  1. I think this was mentioned by another reviewer on another ticket, but my brain trips reading @rtype: L{dict} of L{bytes} -> L{list} of L{bytes}. How do you feel about @rtype: L{dict} mapping L{bytes} to L{list}s of L{bytes}?
  2. Please put the type of something after the description of the thing itself; so @return/@rtype not @rtype/return.
  3. The nesting level in getMessagesToRelay is deeper than I'd like it, but I can't suggest a sensible way to make it less, perhaps because I didn't write this code :)

Some review points:

  1. Spurious ; after break on l437: https://github.com/twisted/twisted/compare/trunk...smarthost-msg-alloc-6717#diff-c8c8e73bb6382f73aba4b74e11a9f2dbR437
  2. The new introduced method, getMessagesToRelay, is public. The only call site appears to be from inside a private method of the class. Unless I'm misunderstanding why this should be public, it should probably be private.
  3. TestRelayQueue docstring documents n, but it's actually called num
  4. TestRelayQueue: https://github.com/twisted/twisted/compare/trunk...smarthost-msg-alloc-6717#diff-7984b244483751d95943dcedfee97f00R1650 the comparison operator needs spaces in between, but maybe just spell it not message.endswith("-D")?

Otherwise, looks good to me :)

Thanks again

lvh

comment:10 Changed 7 years ago by Stacey Sern

Regarding the format for dict type descriptions, I'm fine changing the format to get rid of the arrow. I'm inclined to change L{dict} of L{bytes} -> L{list} of L{bytes} to L{dict} mapping L{bytes} to L{list} of L{bytes} (not L{list}s of L{bytes}). Each mapping goes from one string to one list of strings.

comment:11 Changed 6 years ago by Stacey Sern

Branch: branches/smarthost-msg-alloc-6717branches/smarthost-msg-alloc-6717-3

(In [43036]) Branching to 'smarthost-msg-alloc-6717-3'

comment:12 Changed 6 years ago by Stacey Sern

(In [43037]) Merge forward and resolve conflict in relaymanager.py

refs #6717

comment:13 Changed 6 years ago by Stacey Sern

(In [43038]) Address review comments

refs #6717

comment:14 Changed 6 years ago by Stacey Sern

(In [43196]) Remove trailing whitespace

refs #6717

comment:15 Changed 6 years ago by Stacey Sern

(In [43197]) Eliminate one level of nesting

refs=#6717

comment:16 Changed 6 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

Took care of all the points in comment:9 with the exception of nitpick 2. When I started redoing the mail docstrings I looked at the code and I saw it done this way in some places and followed that example. So now, most of mail has new docstrings in this format. For consistency in mail, I'm inclined to leave it this way.

comment:17 Changed 6 years ago by Stacey Sern

(In [43198]) Fix indentation

refs #6717

comment:18 Changed 6 years ago by Andriy Gapon

Something caught my attention. It's actually not the part of the change, but I wonder why nextMessages list gets reversed before being processed.

comment:19 Changed 6 years ago by Andriy Gapon

Owner: set to Andriy Gapon

comment:20 Changed 6 years ago by Andriy Gapon

Keywords: review removed
Owner: changed from Andriy Gapon to Stacey Sern

I think that I would change the code flow inside the message processing loop a little bit, so that it is clearer.

Review points:

  1. I think that the check that all the connections were exhausted should be moved outside the outermost if-block in the loop. For example, if freeConnections is zero from the very start, then we would still iterate over all the waiting messages without being able to do anything with any of them. In this case we should break out of the loop early.

Nits:

  1. I think that using a single underscore as a name for an ignored return value is not expressive enough.
  2. I would combine the checks before the break statement into a single if statement with an and operator.
Last edited 6 years ago by Andriy Gapon (previous) (diff)
Note: See TracTickets for help on using tickets.