[FS-1148] Backport general changes from mls to develop
#3322
Conversation
zebot
added
the
ok-to-test
mdimjasevic
force-pushed
the
fs-1148/backporting-mls-interface-changes-to-develop
branch
3 times, most recently
from
f69d3ec to
3f869aa
Compare
mdimjasevic
force-pushed
the
fs-1148/backporting-mls-interface-changes-to-develop
branch
from
3f869aa to
962a955
Compare
| MLSMessageResponseUnreachableBackends (Set Domain) | ||
| | -- | If the list of unreachable users is non-empty, it corresponds to users | ||
| -- that an application message could not be sent to. | ||
| MLSMessageResponseUpdates [ConversationUpdate] (Maybe UnreachableUsers) |
There was a problem hiding this comment.
Maybe (NonEmpty ..) instead of just [..] is a bit silly, but ok. :) that discussion doesn't belong into this PR.
There was a problem hiding this comment.
Also, can you explain in the haddocks how users can be unreachable, if not by their backend being unreachable? (If this is obvious to most readers let me know; I have very little context on MLS yet.)
There was a problem hiding this comment.
Maybe (NonEmpty ..)instead of just[..]is a bit silly, but ok. :) that discussion doesn't belong into this PR.
They are isomorphic. I did get some mileage out of my choice over the alternative, but nothing big.
There was a problem hiding this comment.
Also, can you explain in the haddocks how users can be unreachable, if not by their backend being unreachable? (If this is obvious to most readers let me know; I have very little context on MLS yet.)
This comes by extension: if a remote backend is unreachable, all of the users on that backend are unreachable too.
Are you confused by the fact I introduced two data constructors for seemingly the same thing ? In the first one, a commit is considered (therefore, an MLS protocol message) and it either fails (e.g., because some backends are unreachable) or succeeds (all backends are reachable); no partiality allowed. In the second one, an application message is considered and it is fine if it cannot be delivered to some users.
If you can clarify this, that would be great as that would help me understand what should be added to haddocks.
| federationUnreachableError (Set.map domainText -> ds) = | ||
| Wai.mkError | ||
| status | ||
| "federation-unreachable-domains-error" | ||
| ("The following domains are unreachable: " <> (LT.pack . show . Set.toList) ds) |
There was a problem hiding this comment.
| federationUnreachableError (Set.map domainText -> ds) = | |
| Wai.mkError | |
| status | |
| "federation-unreachable-domains-error" | |
| ("The following domains are unreachable: " <> (LT.pack . show . Set.toList) ds) | |
| federationUnreachableError ds = | |
| Wai.mkError | |
| status | |
| "federation-unreachable-domains-error" | |
| ("The following domains are unreachable: " <> (LT.pack . show . fmap domainText . Set.toList) ds) |
| withTempMockFederator' commitMocks $ do | ||
| sendAndConsumeCommitFederated commit | ||
| liftIO $ ftpCommit @?= mempty | ||
| sendAndConsumeCommit commit |
There was a problem hiding this comment.
have you tried translating this test case into /integration? i'd give it a (time-boxed) try after this PR has been merged, /integration is really neat! :)
There was a problem hiding this comment.
I entertained the idea, but many times I need at least three backends that federate, and support for testing that is still in the works.
This change in particular looks like simplifying an existing test, though.
| @@ -745,15 +745,15 @@ notifyConversationAction tag quid notifyOrigDomain con lconv targets action = do | |||
| -- For now these users will not be able to join the conversation until | |||
| -- queueing and retrying is implemented. | |||
| let failedNotifies = lefts notifyEithers | |||
| for_ failedNotifies $ | |||
| logError | |||
There was a problem hiding this comment.
not sure about the log level here. is this something we're doing elsewhere? i think for some clients this may be the right log level, others (more hypothetical ones?) will be hit by a lot of noise. (Yes, this discussion also doesn't belong here, sorry :)
There was a problem hiding this comment.
I don't think we're consistent when it comes to logging, including something like this.
| @@ -532,6 +543,12 @@ postMLSMessageToRemoteConv loc qusr _senderClient con smsg rcnv = do | |||
| MLSMessageResponseProtocolError e -> | |||
| throw (mlsProtocolError e) | |||
| MLSMessageResponseProposalFailure e -> throw (MLSProposalFailure e) | |||
| MLSMessageResponseUnreachableBackends ds -> | |||
| throw . InternalErrorWithDescription $ | |||
There was a problem hiding this comment.
de-duplicate this particular internal error and give it a name?
There was a problem hiding this comment.
... as in to have a new data constructor for InternalError that is suited for this specific purpose?
mdimjasevic
deleted the
fs-1148/backporting-mls-interface-changes-to-develop
branch
|
@fisx , I just merged the PR. However, I am still interested in hearing from you regarding my follow-ups to your questions. Once we sort that out, I'll be happy to make another PR to incorporate that too. |
There are a few things I introduced while working on #3304 after Paolo provided good feedback. These are not specific to MLS, hence this PR that backports them to
develop.Tracked by https://wearezeta.atlassian.net/browse/FS-1148.
Checklist
changelog.d