Cluster-friendly cross-store API

(note #2270)

(note #2467)

Bump

(bump)

(In [15149]) Branching to 'interstore-2191'

(In [15489]) Branching to 'interstore-2191-2'

This has been outstanding for way too long; there's still a bunch of stuff left to do, but I'd like to get a first pass review. The utility module needs to be cleaned up and tested (which is on pjd's plate); there needs to be a convenience API (probably a separate ticket) and there are some obvious flaws documented at the top of the test module (such as the lack of coverage for multiple simultaneous outgoing messages), but I am pretty confident in this outline (and I'm definitely not going to have a chance to even glance at it tomorrow).

That sentence could probably win some kind of "terrible sentence" award, but I think you know what I mean. Let me know what you think.

This is mostly documentation feedback:

• Trivial:
  • spelling error in IMessageReceiver.messageReceived docstring for messageData parameter
  • weirdness in IDeliveryConsequence.answerReceived docstring for contentType parameter
  • IMessageRouter docstring switches to first-person halfway through
  • Redundant articles in IMessageRouter.routeMessage docstring for sender and target parameters
  • QueuedMessage.senderUsername docstring is wrong
  • QueuedMessage.senderDomain docstring is wrong
  • QueuedMessage.senderShareID has no docstring
  • MessageQueue._scheduleMePlease is named in the first person and has a docstring written in the first person
• Minor:
  • xmantissa.messaging module docstring starts off with a justification of an axiom feature. I feel like this text would be better off somewhere else (perhaps axiom.userbase or axiom.userbase.LoginSystem or in a document which isn't a docstring at all), with a reference to it in xmantissa.messaging. To some extent, this is the problem of ensuring the consumer of the documentation has all the pre-requisites to understand it. Including all (or even some) the pre-requisites in each document isn't a solution. :) The discussion of the limitations of the axiom feature don't seem as out of place here.
  • The brief mention of broadcast in the xmantissa.messaging docstring should probably be expanded or removed (perhaps into the description of a new ticket). It's not really clear what it means and it will probably just confuse people.
  • DeliveryConsequence.answerReceived docstring doesn't really say anything anyone might want to know.
  • AlreadyAnswered class docstring is a bit light.
  • Some attributes of AlreadyAnswered are missing docstrings.
  • Docstrings for LocalMessageRouter don't explain what's interesting about this particular implementation, they mostly just re-iterate information from the interface (by either referring readers to it or restating docstrings from the interface).
  • MessageQueue.routeMessage docstring is confusing. How many possible users are there in the store?
  • MessageQueue._deliverAnswer has no docstring
• Significant:
  • xmantissa.messaging suggests that applications passing messages between stores need to select a well-known string (such as "calendar") to use as a message target. This looks like a recipe for conflict. Shouldn't we at least be suggesting strings more likely to be unique? "com.divmod.calendar.v1" or "922d516e-2e16-4b9d-ab3f-0ba945cd7597"?
  • The implied majortype/minortype convention for contentType doesn't seem like something worth copying from MIME. Even if we do, it needs to be cleaned up a lot. Who gets to assign types? Is case significant? Does Mantissa define any other than error? I particularly like how a content type of e3rror/database-lost will be delivered as a success response by DeliveryConsequence. :)
  • How much of the interface of xmantissa.messaging is actually public? It looks like all the classes are, but it doesn't seem like QueuedMessage needs to be, nor AlreadyAnswered, nor MessageQueue.

I guess that's enough of a review for now.

that diff we were looking at

(In [15652]) deal with all review feedback in the 'trivial' section.

re #2191

(In [15653]) (attempt to) deal with all review feedback in the 'minor' section.

re #2191

(In [15654]) make more things private, as suggested by 'major' feedback section

re #2191

As far as MessageQueue and being private goes, it seems to me like it's a thing some powerups are going to want to depend on, and MessageQueue.queueMessage is a thing that some powerups are going to want to call. So I think it's low-level, but public; this is a level we may want to tweak some behavior at the application level in later.

MIME stuff was already dealt with in some earlier revisions - the only point left to address is the "well-known string" thing.

Replying to glyph:

As far as MessageQueue and being private goes, it seems to me like it's a thing some powerups are going to want to depend on, and MessageQueue.queueMessage is a thing that some powerups are going to want to call. So I think it's low-level, but public; this is a level we may want to tweak some behavior at the application level in later.

and, by way of an example, it occurs to me that the attached patch does exactly that...

(In [15655]) apropos of remaining 'major' review feedback, forego complex solution ("922d516e-2e16-4b9d-ab3f-0ba945cd7597") to avoid conflicts, just avoid providing aggressively bad advice in the module docstring

re #2191

OK. That should be all for the review feedback, but there's still the remaining to-do items in the test module docstring and the _recordattr cleanup from pjd.

(In [16167]) Branching to 'interstore-2191-3'

FOR GREAT JUSTICE

• axiom/dependency.py
  • missing copyright statement
  • some missing whitespace between stdlib and 3rd party imports
  • class line for requiresFromSite is >80 cols
  • can we standardize on 4 space indentation for the subsequent lines of epytext @ markup?
  • requiresFromSite.siteDefaultFactory is documented as being called with the site store, but I think it's actually called with something else
• axiom/test/test_dependency.py
  • missing copyright statement
  • 3rd party imports incorrectly follow twisted imports
  • NullGrid class docstring slips into the 1st person briefly
  • RealGrid.powerupInterfaces is a list, should be a tuple; mutating it will never do a good thing.
  • RealGrid.totalWattage has no doc string
  • IronLung.wattsPerPump has no doc string
  • IronLung.grid is not documented - I guess it needs an @ivar in the class docstring, since requiresFromSite doesn't take a doc parameter.
  • the class docstring for PowerStrip isn't formatted properly
  • SpecifiedBadDefaults dummy and grid attributes have no doc strings
  • PowerStrip.grid has no doc string
  • PowerStrip.draw has no docstring
  • PowerPlant's two attributes have no doc strings
  • requiresFromSite is really quite separate from the rest of the dependency module, at least as far as its implementation goes. It might make some names a bit nicer if its tests were organized onto a different TestCase instead of being part of DependencyTest.
  • test_requiresFromSiteOnlySite looks only about half written
  • the last two assertions in test_requiresFromSiteNoDefault seem to cover the same thing; the comment before the last one seems unrelated to what the last one actually is
  • test_requiresFromSiteUnspecifiedException uses failUnless, should use assertTrue. Except, I'm not really sure why UnsatisfiedRequirement subclasses AttributeError. I can kind of almost see how it might make sense (you tried to get the grid attribute, but no suitable object could be found, so it's like there's no attribute) but I'm not sure. Maybe if the docstring for UnsatisfiedRequirement were a bit beefier this wouldn't be a problem. Or maybe it still would be. :)
• axiom/userbase.py
  • no copyright statement
  • extra blank line at the top of the module docstring
  • 3rd party imports incorrectly follow twisted and axiom imports
• Epsilon/epsilon/expose.py
  • test-case-name is too specific
  • missing copyright statement
  • missing module docstring
  • MethodNotExposed has an empty class docstring
  • key parameter to Exposer.expose is undocumented
• Epsilon/epsilon/test/test_expose.py
  • no copyright statement
  • hooray, a module docstring - in test docstrings, I've been describing the thing being test more lately, rather than just naming it, but I think this one goes over the top a bit, particularly since it doesn't say anything about testing, and epsilon.expose. A lot of its text could probably go into epsilon.expose's docstring.

Guess the branch is too big. :) I'll continue on this review tomorrow morning or maybe later today if I feel like I can concentrate hard enough again.

Replying to exarkun:

missing copyright statement

I've updated these, but if they're really important we might want to have a pass that adds them. Almost no files in Axiom contain copyright statements.

can we standardize on 4 space indentation for the subsequent lines of epytext @ markup?

I can't remember if I have ever done that. It seems like I've always done it the way I do now... it makes reflowing with emacs more of a pain, and I don't particularly see the benefit. Although I notice the epytext examples do it with a 4-space indent, so maybe when I originally read them I did it that way. More importantly, I don't feel like reflowing every docstring in this branch :). So can we have that discussion later?

requiresFromSite.siteDefaultFactory is documented as being called with the site store, but I think it's actually called with something else

Nope. That's the site store. Note the way it's invoked: only in the case where the item in question is in the site store. Or at least, is in the site store as best the inspecting code can tell; it's possible to open the user store without a parent so that it looks like a site store, but that case is accounted for differently. test_requiresFromSiteInSiteStore (previously test_requiresFromSiteOnlySite) verifies this with its .siteStore assertions about the returned NullGrid.

NullGrid class docstring slips into the 1st person briefly

Heh. That was me, not knowing where your power comes from, if you are using a NullGrid, not NullGrid wondering about itself. The sentence was pointless though, so it's removed.

RealGrid.powerupInterfaces is a list, should be a tuple; mutating it will never do a good thing.

RealGrid.powerupInterfaces = somethingElse wouldn't do a good thing either, but we don't bother to make the type immutable... I'm leaving it as a list, because I think screwing up (IFoo,) syntax is a more likely class of error than accidentally calling powerupInterfaces.append. I have definitely seen some interesting and difficult-to-diagnose errors crop up thanks to the fact that interfaces are themselves iterable.

I'm not really sure why UnsatisfiedRequirement subclasses AttributeError.

One word: getattr. I've updated the test itself to use that as the criterion, though, so it should be much clearer.

Epsilon/epsilon/test/test_expose.py hooray, a module docstring - in test docstrings, I've been describing the thing being test more lately, rather than just naming it, but I think this one goes over the top a bit, particularly since it doesn't say anything about testing, and epsilon.expose. A lot of its text could probably go into epsilon.expose's docstring.

I have been trying to write docstrings more and more for input into something like that 'testdoc' hack that jml wrote a while ago. That suggests to me that we should never really talk about testing, just about the properties and systems being tested. That does sort of beg the question of why have test module or TestCase subclass docstrings at all, rather than just a reference to the module being tested, but I have been using them as a chance to give brief summaries rather than expansive descriptions.

Given that expanded description of why I did it that way, do you still think it should be changed?

Guess the branch is too big. :) I'll continue on this review tomorrow morning or maybe later today if I feel like I can concentrate hard enough again.

I hope this round of feedback has been adequately dealt with ... time to do the next one?

(In [16179]) re #2191 deal with phase 1 review feedback

Some more mostly minor things:

• typo in requiresFromSite docstring - callabel
• the @return for Expose.expose is pretty confusingly worded
• in the docstring for IDeliveryConsequence, In order to provide this interface an item must also be an item - item redundancy
• The first sentence of the docstring for answerReceived is also confusing
• Types of the arguments to answerReceived aren't documented, I assume they're the same as the similar parameters to IMessageReceiver.messageReceived. Maybe it would be useful to combine type and message into a single type with a couple attributes.
• The Note in IMessageRouter's class docstring would probably benefit from a ticket number.
• Missing whitespace around operators in BadSender.__init__
• inconsistent whitespace for parenthesis in import statements in interstore.py
• we can probably spare a few locals to be used in LocalMessageRouter.routeMessage and LocalMessageRouter.routeAnswer
• _accidentalSiteRouter does a really awful thing with the logging system - it passes an exception instead of a failure and it doesn't supply a descriptive string explaining the error. This will result in difficult to track down errors in the log file.
• _createLocalRouter does the same thing
• MessageQueue.routeMessage also calls log.err without a descriptive string.
• MessageQueue._deliverAnswer also has an expression which could be shortened
• MessageQueue.routeAnswer calls log.err without a string (twice)
• whitespace in MessageQueue._verifyAnswer
• AMPReceiver.messageReceived has an XXX in it
• test_interstore.py has two or three XXXs in it

Probably will have some more comments later. This is a ton of new code and functionality with lots of subtleties and it will take a while to sink in.

(In [16203]) Respond to review feedback.

re #2191

I'm sure the Trac markup is totally screwed on most of this, I apologize in advance:

• the @return for Expose.expose is pretty confusingly worded
  • I tried to improve it (along with the rest of the peculiarly-worded docstring), but I'm not sure I succeded. Do you have any suggestions?
• The first sentence of the docstring for answerReceived is also confusing
  • Similarly, I tried to provide more detail, but I don't know if it's less confusing. These concepts are crystal-clear in my mind but I seem to be having trouble communicating them.
• Types of the arguments to answerReceived aren't documented, I assume they're the same as the similar parameters to IMessageReceiver.messageReceived.
  • I've done this, but...
• Maybe it would be useful to combine type and message into a single type with a couple attributes.
  • I've almost done this a few times. Before finally merging I still might want to do it. I'd really like your thoughts on this, though: it seems weird to have an object with just two attributes, and no methods (nor, as far as I can see, the possibility of methods, since everything that's acting on message data has to do some parsing first). Do you think it makes sense to include 'sender' and 'target' attributes as well? If so, how would they look in answerReceived? If not, we still have to pass around sender/target as a unit with the message (no code I can think of can process one without at least potentially needing the other).
• The Note in IMessageRouter's class docstring would probably benefit from a ticket number.
  • Oops. The higher-level API it was talking about actually exists now! I've adjusted the suggestion to point to it first.
• we can probably spare a few locals to be used in LocalMessageRouter?.routeMessage and LocalMessageRouter?.routeAnswer
  • Not really sure what you mean by this. Reading the code I don't see any repeated expressions that could be eliminated.
• _accidentalSiteRouter does a really awful thing with the logging system - it passes an exception instead of a failure and it doesn't supply a descriptive string explaining the error. This will result in difficult to track down errors in the log file.
  • I've adjusted it (and _createLocalRouter, and routeMessage, and routeAnswer, which all have the same problem) to have a raise/except so that stack frames can be generated and examined in a debugger context. I also added some messages. However, I have no idea how (or even if I should) update the tests to verify these details. Is this the sort of thing you had in mind?
• MessageQueue?._deliverAnswer also has an expression which could be shortened
  • I am guessing you are referring to the right-hand side of the deliveryDeferred assignment. I don't see how it could be shortened, except by moving the addCallbacks to another line, which would be more code overall. Why would that be desirable?
• test_interstore.py has two or three XXXs in it
  • Since I just deleted these, I should explain: the remaining two None arguments are verified by the SenderArgument and TargetArgument tests, and the disambiguation between different error types was done with the addition of specific error codes for ERROR_NO_SHARE, ERROR_NO_USER and so on.
• AMPReceiver.messageReceived has an XXX in it
  • I haven't dealt with this yet, I need to write a few tests for malformed input of various kinds. I'll do that in a separate commit.

(In [16204]) Respond to remaining review feedback about XXX's.

re #2191

I've almost done this a few times. Before finally merging I still might want to do it. I'd really like your thoughts on this, though: it seems weird to have an object with just two attributes, and no methods (nor, as far as I can see, the possibility of methods, since everything that's acting on message data has to do some parsing first). Do you think it makes sense to include 'sender' and 'target' attributes as well? If so, how would they look in answerReceived? If not, we still have to pass around sender/target as a unit with the message (no code I can think of can process one without at least potentially needing the other).

I don't think it's any weirder in this case than than it is in any other case where we use epsilon.structlike.record. I don't think this is a major conceptual issue, I just think it's an ease of use and ease of comprehension thing: a message is always a type and a body; providing these two things separately doesn't provide any utility and expands parameter lists, introduces the possibility of mixing them up, and otherwise makes it harder to use this interface.

As for the sender/target attributes, that might make sense as well. What I almost also suggested last time was combining those two into their own object. It probably makes sense most of the time to have all four of these combined, but the coupling here seems slightly weaker. I may want to represent a connection (that's almost what a sender/target pair is) independently of any particular message so that I can send multiple messages using those parameters; likewise, I may want to send one message to or from multiple addresses. So I think rolling sender/target into one object would be an improvement as well, but rolling all four into one may not be.

Not really sure what you mean by this. Reading the code I don't see any repeated expressions that could be eliminated.

Sorry, I meant this:

Index: Mantissa/xmantissa/interstore.py
===================================================================
--- Mantissa/xmantissa/interstore.py    (revision 16207)
+++ Mantissa/xmantissa/interstore.py    (working copy)
@@ -371,8 +371,8 @@
             router.routeMessage(sender, target,
                                 messageID, messageType, messageData)
         else:
-            self._routerForAccount(
-                sender.localpart, sender.domain).routeAnswer(
+            router = self._routerForAccount(sender.localpart, sender.domain)
+            router.routeAnswer(
                 sender, target, messageID, DELIVERY_ERROR, ERROR_NO_USER)
 

A similar change in routeAnswer would be an even bigger improvement. Basically, I don't think 5 line statements are good for readability. There's little repetition, but the spacing gets weird and there are too many variables involved. Of course, if the signature for routeAnswer is shortened to connection, messageID, message (connection isn't a great name for that parameter, but nothing better comes to mind at the moment; also now I wonder if a message object shouldn't also include a messageID along with its type and body) then this concern might be lessened.

• xmantissa.interstore
  • an __all__ would probably be nice. We like these, right? If so we should try to use them more often.
  • the module docstring talks Target, but I think it should be talking about Identifier now?
  • It also talks about how utilities for treating message bodies as AMP boxes are forthcoming, but I think you wrote a lot of them already.
  • the new try/raise/except dealies are interesting. That's not what I had in mind. I mentioned the lack of tracebacks just as part of the description of the problem - no traceback and no description makes it hard and sometimes impossible to know where the error came from. Adding tracebacks fixes is, but so does adding descriptions. You did both, so I guess there's no chance of being unable to find the location of errors now. :) The tracebacks do add a bit, but they're mostly noise - I think in almost all cases, all the code they point to is framework code that no one will care about. OTOH, maybe we'll care about it if we need to debug stuff near this. So I guess it's good; we can always take away some information later if it is hurting something.
• xmantissa.test.test_interstore
  • flushLoggedErrors takes a type argument so that you don't have to call trap on the results. Are you avoiding this feature intentionally?

Another minor API comment. DeliveryConsequence could be a wrapper instead of a mixin, and containment is better than inheritance, right? Oh, but in most usage this will have to be an Item, so there'll be no opportunity to apply the wrapping, unless IPowerupIndirector is used, and then it's a lot more work to use this helper, unless there's a mixin that defines the appropriate indirect method. Guess there's no way to win here. I wonder if there's something in general we should do to make inheritance less necessary?

I'm going to take a look at some application code that uses this and see how it feels.

(In [16211]) Thanks a lot for the detailed description of what you were looking for, JP. I messed around with it a bit, and this seemed to look best.

I've added an interstore.Value which encapsulates just a type and data. It doesn't really do much for the code, but it makes the documentation a heck of a lot easier to read and write. More coupling didn't really seem to make sense.

I've (hopefully) removed the longish expressions. Also, since I was adjusting all these parameter lists, I moved the order around to be a bit more consistent between routeMessage and routeAnswer.

re #2191

(In [16212]) Remove DeliveryConsequence, since it was a bad idea anyway.

re #2191

• DeliveryConsequence? could be...
  • DeliveryConsequence was actually a fairly dumb idea, now that we have both high-level and low-level APIs; it provided an intermediary layer which wasn't used in implementing the high level. I've deleted it. Of course, your comments apply equally well to AMPReceiver, but I don't know what we can actually do about it - at least one mixin is better than two. I think it'll be pretty trivial to introduce some indirection, if we ever figure out how to do that in a nice, general way.

I think I've dealt with everything...

Regarding the listness of the powerupInterfaces attribute of certain classes: over the entire codebase, there are about 90 instances where this attribute is bound to a tuple; there are 4 where it is bound to a list. You're right that it can be changed regardless of what type it is. Looking at it fresh, I would say that a list makes more sense (coming from the school of thought where a list is a homogeneous collection and a tuple is a heterogeneous one). However, I would still argue in favor of consistency for pedagogical reasons, and making everything a tuple is a smaller change than making everything a list.

The nested function destroyAnswer in MessageQueue._deliverAnswer calls Item.deleteFromStore outside a transaction. It's always deleting an _AlreadyAnswered item, so perhaps that's always an atomic operation. Is it necessarily? Presumably the case where a third party creates a whenDeleted=CASCADE reference to one of these is not supported. Still, Axiom will do a bunch of queries to check for cascading reference-to-anys. Probably not a big deal, but probably not exactly correct. On the other hand, concurrency isn't supported. Should I even bother trying to check for transactional correctness? I think there are probably a few other places that might benefit from adjustment, if so.

Also in MessageQueue._deliverAnswer, the Deferred which isn't returned also isn't given a catch-all errback, though it is careful to trap only a particular exception type earlier. Is that trap tested (sorry, I should go look through the tests to answer that question myself, but it would be hard - I can imagine a tool which would answer this question for me, perhaps I will try to implement it)? Is the negative path for the trap tested? If another exception type is possible, there should be at least a log.err as the final errback to ensure timeliness.

I've been thinking about the replication implications of this branch. It'd be good to hook up in realtime or meatspace and discuss them at some point.

(In [16222]) I still really think this ought to be a list, but I'll change it for now. I feel pretty strongly that we should use lists though (the failure mode of forgetting the "," in "(IFoo,)" is terrible), and I'll file a ticket for making that change.

re #2191

(In [16223]) Turns out that this is untested...

re #2191

Replying to exarkun:

The nested function destroyAnswer in MessageQueue._deliverAnswer calls Item.deleteFromStore outside a transaction. It's always deleting an _AlreadyAnswered item, so perhaps that's always an atomic operation. Is it necessarily?

In attempting to write a test for this, I discovered some very interesting issues in the area of deleteFromStore. For example, there's no way to declaratively "you aren't allowed to have an item which points to a _QueuedMessage or an _AlreadyAnswered which could prevent its deletion". The item names are private, nothing establishes references to these items which might interact with their deletion, and anything external doing so would be in error. However, I could write a test that would have an item that creates another item in its deleteFromStore and then raises an exception. However, there's no actual correct thing to do in that situation: the _AlreadyAnswered won't get deleted, so it will continue to be scheduled for retransmission, which is wrong.

I'd like the code here to be transactionally correct (and there are some tests to verify the aspects of that which are publicly visible) so if you see anything else, let me know.

Also in MessageQueue._deliverAnswer, the Deferred which isn't returned also isn't given a catch-all errback, though it is careful to trap only a particular exception type earlier. Is that trap tested?

Nope. In the commit above, I noticed that commenting the trap out causes the tests to pass. I clearly need a bit more test coverage here.

I've been thinking about the replication implications of this branch. It'd be good to hook up in realtime or meatspace and discuss them at some point.

Hopefully I'll see you tomorrow to do that. I was hoping this branch wouldn't have any replication implications, though...

To complete my thoughts on _deliverAnswer: goes, I do think we should do something about it but I think that given the number of issues with deleteFromStore and the fuzziness around its semantics, we should do it in a subsequent ticket.

(In [16224]) Per review feedback, add a test for unexpected error types, and an end-of-the-line error logger.

re #2191

Modulo discussion of replication concerns, I think that's it.

Hopefully this is the last of it. There's a lot, but of the parts that aren't trivial, I don't know how much of it is work that actually must be done before this is merged. A lot of it might make sense to split off into separate tickets.

• Mantissa/xmantissa/sharing.py
  • Identifier.fromSharedItem docstring
    • L{cls} should be C{cls} I think - you can't link to a parameter in epytext, as far as I know.
    • The API isn't really non-deterministic. Perhaps unstable? alterpotent? (too bad that's not a real word) It certainly doesn't do anything at random, it just doesn't bother to try to provide any high-level precedence to the possible results. Say, maybe it should? Even something as boring as giving back the share with the "smallest" shareID for the role with the "smallest" name? Since apparently we will be using this API, despite its shortcomings, it'd be nice if it behaved in the least surprising way possible. Actually, forget shareID and name, how about smallest storeID?
    • It's weird to be introducing a new API with a docstring that re-assures the reader that it isn't deprecated. I also think the predictions about the future don't add much. Either we won't ever deprecate it, in which case the users who have been forced to use it (since there is nothing else in many cases yet) won't notice or care, or we will in which case the prediction is wrong.
    • {Identifier.fromSharedItem} appears in the docstring text - some qualifier is missing.
• Mantissa/xmantissa/test/test_sharing.py
  • Are we really going to have enough APIs which don't give necessarily correct results to justify a whole test case with Heuristic in its name? :) At first I missed the test for getSelfRole here. Now I see it's the only direct test for getSelfRole in the whole suite (although a few other tests do depend on it, though to what extent I didn't bother to check). Should getSelfRole really be heuristic? I suppose it is now - given multiple internal login methods, none is more or less correct than the others. Here's a thought - make the self role independent of internal login methods, and have a login method role of some sort, all of which have the independent self role as a member. Oh, but my point was about the name of the test case. Maybe it could be a test case for Identifier.fromSharedItem and be named as such, and the getSelfRole test could move to a different test case and get some friends.
  • I think IdentifierTestCases should mostly use assertTrue and assertFalse instead of assertEquals, assertNotEqual, failIf, etc.
  • test_argumentSerialization is at least two different tests
    • correct serialized bytes from an IdentifierArgument
    • ability to parse serialized format
• Mantissa/xmantissa/error.py
  • BadSender doesn't document its ivars
• Mantissa/xmantissa/ixmantissa.py
  • IMessageReceiver.messageReceived still returns a two-tuple, should it be a Value now?
  • The third paragraph of the IMessageRouter docstring says code should be always be using
  • The last part of the third paragraph says different providers may only know about routing to specific locations - I don't understand this; there's the local-only router, and presumably there'll be an exponent router someday, is this the different it is talking about? If so, how does the use of a higher-level API make any difference?
  • The type of the value parameter to IMessageRouter.routeMessage is undocumented
  • The value parameter to IMessageRouter.routeAnswer is entirely undocumented
• Axiom/axiom/dependency.py
  • These changes are probably going to conflict with #2656, I think
• Axiom/axiom/errors.py
  • it'd still be nice to have more of a docstring for UnsatisfiedRequirement
• Mantissa/xmantissa/test/test_interstore.py
  • some twisted imports follow xmantissa imports
  • Receivomatic has some somewhat non-trivial behavior involving its attributes; they should be documented.
  • Same for StubSlowRouter and for the couple attributes of StubDeliveryConsequence which aren't documented
• Mantissa/xmantissa/interstore.py
  • MessageQueue.run has a no-argument log.err call
  • A couple things I notice about the local-only router:
    • errors in answerReceived will need administrative attention to resolve; answers will stack up indefinitely otherwise
    • time-based scheduling doesn't seem necessary or desirable; is this just for simplicity of implementation? All the information is local, so it'd be possible to do everything exactly as soon as it could be done and not do the polling thing at all. Maybe that's more work to implement though.
    • when there's a cross-process version of this, polling won't necessarily be desirable either - it should be possible to know exactly when an attempt to do something might succeed and when it will definitely fail.

(In [16295]) re #2191: deal with all review feedback except the answerReceived error-handler

(In [16314]) re #2191 - Remember failed attempts to process answers for future processing.

(Also, rename cheezy "receivomatic" test stub.)

(In [16315]) Branching to 'interstore-2191-4'

Replying to exarkun:

Are we really going to have enough APIs which don't give necessarily correct results to justify a whole test case with Heuristic in its name? :) At first I missed the test for getSelfRole here. Now I see it's the only direct test for getSelfRole in the whole suite (although a few other tests do depend on it, though to what extent I didn't bother to check). Should getSelfRole really be heuristic? I suppose it is now - given multiple internal login methods, none is more or less correct than the others. Here's a thought - make the self role independent of internal login methods, and have a login method role of some sort, all of which have the independent self role as a member. Oh, but my point was about the name of the test case. Maybe it could be a test case for Identifier.fromSharedItem and be named as such, and the getSelfRole test could move to a different test case and get some friends.

For now, I think this is the best thing to do, since these are heuristics at the moment. Their results are correct, they're just returning one thing from an unordered set of correct answers. In particular, getSelfRole should really have some tests in its current form before we change it :). But I've filed tickets to get rid of them in separate phases.

I like your idea about getSelfRole. I've filed #2665 to implement it, since it's almost entirely unrelated to this ticket.

The API (fromSharedItem) isn't really non-deterministic. Perhaps unstable? alterpotent? (too bad that's not a real word) It certainly doesn't do anything at random, it just doesn't bother to try to provide any high-level precedence to the possible results. Say, maybe it should? Even something as boring as giving back the share with the "smallest" shareID for the role with the "smallest" name? Since apparently we will be using this API, despite its shortcomings, it'd be nice if it behaved in the least surprising way possible. Actually, forget shareID and name, how about smallest storeID?

I've re-worded the docstring to remove all the prevarication and explain more clearly what it does. Making it be deterministic means re-implementing or re-specifying getAccountNames, which is a bunch of extra work and I don't think it should delay this branch. I have filed #2666 for this work.

These changes are probably going to conflict with #2656, I think

I've merged forward, but you probably want to have a look at the revisions I've linked to in my previous comments if you want to make any comments on the review.

time-based scheduling doesn't seem necessary or desirable; is this just for simplicity of implementation? All the information is local, so it'd be possible to do everything exactly as soon as it could be done and not do the polling thing at all. Maybe that's more work to implement though.

when there's a cross-process version of this, polling won't necessarily be desirable either - it should be possible to know exactly when an attempt to do something might succeed and when it will definitely fail.

The scheduling algorithm is far from optimal, but it basically works. I don't think that we should start tweaking it until we have an actual networked implementation.

I've filed #2674 for this work.

errors in answerReceived will need administrative attention to resolve; answers will stack up indefinitely otherwise

It was actually worse than that; the messages were stacking up indefinitely and therefore being retransmitted infinitely. I've altered the system to allow the conversation to come to a close, delete the _QueuedMessage, and record the buggy answer. I also filed #2675 for adding a tool to allow administrators to perform the necessary action.

• Should IElectricityGrid have a draw method?
• Should NullGrid and RealGrid declare that they implement IElectricityGrid?
• RealGrid.powerupInterfaces is a list (as discussed above, it should be a

tuple for now)

• _NullRouter.routeAnswer and the if qm is None: check in MessageQueue.routeAnswer aren't tested

Merge when fixed.

(In [16327]) re #2191 IElectricityGrid should have a draw method

(In [16328]) re #2191 NullGrid and RealGrid should declare that they implement IElectricityGrid

(In [16329]) re #2191 RealGrid.powerupInterfaces should be a tuple for now

(In [16330]) re #2191 _NullRouter.routeAnswer should be tested.

(In [16334]) re #2191 There should be a test for repeated answer deliveries (the 'qm is None' case).

(In [16342]) Add an API for messaging between stores of Mantissa users.

Fixes #2191

Author: glyph

Reviewer: exarkun

The new xmantissa.interstore module allows applications to queue messsages for transactional delivery between different users. This mechanism is pluggable, so that applications which use it can transparently use a networked (i.e. clustered) transport for these messages.

This new facility also provides a convenience layer for interpreting message bodies as AMP commands, and dispatching them appropriately to different methods depending on the command type and registered responders for those commands.

There were also a few APIs added in support of this change.

• axiom.dependency.requiresFromSite allows Axiom items to declare "soft" dependencies on powerups in the site store, which can be satisfied by installing powerups, or at runtime. This allows applications to gracefully refer to facilities which may be stateful if they are configured, but functional even if no configuration is present.

• epsilon.expose provides an API for creating decorators that expose methods over particular protocols. This is a good definitive implementation of this functionality: unlike the version in twisted.protocols.amp, it does not require the use of a metaclass (and is not specific to AMP). This allows it to be used by Item subclasses, or other classes which have their own metaclass requirements. Unlike the version in nevow.util, it allows for dispatch on keys other than the method name, allowing method names to be decoupled from protocol-level identifiers.

• xmantissa.sharing.Identifier provides a helpful, way structured to pass around the bundles of username/domain/shareID that are very common in dealing with code in the sharing system. Additionally, a new utility method, Identifier.fromSharedItem, provides a way for applications that use sharing to ask how an item is referred to, if it is already shared.