Rework partial sends and receives #200

tfpauly · 2018-06-24T23:02:09Z

Satisfies #148

Support partial sends more naturally by splitting out into Message, messageData, and endOfMessage flag.
Do the same for Receive
Receive should also take minIncompleteLength to specify how partial receives are treated (or not, by default)

chris-wood

Overall, this looks good. I mostly have editorial questions.

chris-wood · 2018-06-25T02:06:28Z

draft-ietf-taps-interface.md


-The Message Object passed to Received is complete and atomic, unless one of the following
+If the minIncompleteLength was set to be infinite, the Message passed to Received
+will complete and atomic (that is, endOfMessage will be set), unless one of the following


s/will/will be

chris-wood · 2018-06-25T02:06:28Z

draft-ietf-taps-interface.md


-The Message Object passed to Received is complete and atomic, unless one of the following
+If the minIncompleteLength was set to be infinite, the Message passed to Received
+will complete and atomic (that is, endOfMessage will be set), unless one of the following


s/that is/i.e.?

i.e. literally means "that is", so I think this is about the same?

chris-wood · 2018-06-25T02:06:28Z

draft-ietf-taps-interface.md

@@ -945,29 +945,46 @@ capacity that it sees fit.
 # Sending Data {#sending}

 Once a Connection has been established, it can be used for sending data. Data
-is sent by passing a Message Object and additional parameters
-{{send-params}} to the Send Action on an established Connection:
+is always sent in association with a Message Object and additional parameters


Should we use something other than association here? That may complicate references to association elsewhere.

“bound to” maybe? “Together with”?

chris-wood · 2018-06-25T02:06:28Z

draft-ietf-taps-interface.md

+Message := NewMessage()
+~~~
+
+The messageData parameter contains any octects to be sent for this message.


Remove "any"?

chris-wood · 2018-06-25T02:06:28Z

draft-ietf-taps-interface.md

-can be added to that Object. A sendParameters Object can be reused for
-sending multiple contents with the same properties.
+Parameters may be added to a Message object before the Message is used
+for sendings.


Maybe comment that message parameters cannot change after Send is invoked?

I'm wondering how this works with, e.g., "Final" messages. If Message Properties cannot be changed after Send, the application cannot reuse a Message object with an additional "Final" parameter added, but needs to create a new Message object just for this purpose, adding all properties of the previous messages to it, and "Final". Is this intended?

If you need to do a write close with Final, you should mark that on your Message in the first send. If you need to send a partial Message that isn't necessarily the Final one, then don't mark it Final. You can later send a complete message that is final with no data.

@chris-wood Added text

chris-wood · 2018-06-25T02:06:28Z

draft-ietf-taps-interface.md

@@ -1185,30 +1193,41 @@ number of Messages with a single call. This allows an application to provide
 backpressure to the transport stack when it is temporarily not ready to
 receive messages.

+Receive takes an optional minIncompleteLength argument. This value indicates


If it's optional, it should be the last parameter, or we should specify that a value of 0 is the default when one should use it.

in general the question of defaults should be better addressed in this draft. I think we can say that “syntactic sugar is allowed, here are the default behaviors we recommend”. Everything about partial send should be optional. This also implies we need connection-bound default send parameters...

Both min and max are optional, so they can't both be the last parameter?

Yep -- I made the comment before getting to the next paragraph. Maybe add: "Both minIncompleteLength and maxLength are optional parameters." at the beginning of this paragraph, and then proceed to describe them both?

chris-wood · 2018-06-25T02:06:28Z

draft-ietf-taps-interface.md

+value is infinite, which means that only complete, atomic Messages will be delivered.
+If this value is set to some smaller value, the associated ReceiveHandler will
+be triggered only when at least that many bytes are available, the Message is
+complete, or the system needs to free up memory.


Since the last case is exceptional, perhaps we should mention that applications must check the received length before use, as it may be less than minIncompleteLength?

chris-wood · 2018-06-25T02:06:28Z

draft-ietf-taps-interface.md

+protocol, and provides methods to access metadata about the received data.
+The octets of data associated with this message are delivered as messageData.
+Multiple invocations of the ReceivedHandler may devlier data for the same
+Message until the endOfMessage flag is delivered. See {{receive-framing}} for 


Partial message data is sent in-order. Received data may be received out-of-order, right? Do we need to clarify that?

This is THE problem I have with this whole thing - it's also the question that I raised at the end of #148 and don't see addressed. Partial messages may arrive out of order, or they may never arrive at all, if we map them on an unreliable transport. At least, applications must have a number to identify that they're now getting message fragment 3 out of 7, something like that.

And what about partial reliability? E.g., fragments 1 and 2 out of a total of 3 arrive in time, but for the third, the time expires and the transport gives up. Shouldn't it then tell the receiving application that it will never get the third fragment, and should throw away the first two?

My inclination here is to only receive chunks in order and not deliver gaps. Does that work for you?

@tfpauly I think that's a good direction, as it makes things much easier. I think we should then still add a way for the transport system to signal to the application: "throw away your chunks, the rest is never going to arrive" - e.g. to support partial reliability.

But I have a bigger concern, then: what about UDP? What if I send a large message, the chosen transport is UDP, and the message is split it into 3 packets - first, this is perhaps not a great idea; second, the receiver won't be able to know about the order, so can't even do what you suggest here. How are you going to handle this case?

So, in our implementation, we allow the protocol to require that data for a single message fits within a limit (for UDP, whatever it can fit in a datagram). Splitting up a message over multiple UDP datagrams without any other framing layer on top doesn't make any sense, since there isn't any way for the other side to understand the message. So, in our implementation, we throw an error if you try to do this.

Protocols on top of UDP, like IKE, may support fragmenting large messages across datagrams, but then the message that you send is first interpreted by a protocol in the stack that supports unbounded message sizes.

I also do agree that there should be a signal that the chunks won't ever arrive—our implementation passes an optional error along with the ReceiveHandler to indicate this.

Ah ok! Great, I think that solves it. I'll conclude by requesting the following text:

in the API draft, some kind of a signal that the chuncks won't error arrive (as we agreed)

in the API draft, for the receiver side, a hint that message chunks will only arrive in order

in the implementation draft, a hint that message fragmentation of course requires support by the underlying protocol and thus the size of any message, even with fragmentation, will have to be limited to the message size that the underlying protocol supports. For example, messages can not be spread across multiple UDP packets because the receiver would not understand that the message parts belong together.

I can take care of 2 and 3 (but I'm fine with you doing it too if you want! Just offering to help instead of always only criticizing!) .... but for 1), I think it would be better for you to do it since your implementation already does it. Ok?

theri · 2018-06-25T07:55:55Z

Looks like a good approach.
I have a general question about setting Message Properties:
This Pull Request gets rid of an explicit MessageProperties object and instead sets any Message Properties directly on the Message. For (Pre-)Connections, we do have an explicit TransportProperties object, which has the advantage that it can be reused across multiple Connections without being tied to any specific Connection. I think that the same would be useful for Messages, e.g., across multiple threads.
What is the advantage of setting properties directly on a Message, versus having an explicit MessageProperties object, which is then assigned to the Message?

britram

thanks @tfpauly for doing this... this seems to be the simplest way to handle message non-atomicity while still providing for atomic(-like) sends and receives in the general case. All of my problems with what's here are about the elegance of the interface the programmer will see (a "Message" that is separate from the content seems misnamed; this adds complexity that we should make sure everyone knows can be papered over with syntactic sugar in the common case).

I'm happy with landing this as-is for Montreal and filing an issue to discuss in person, though.

britram · 2018-06-25T08:51:43Z

draft-ietf-taps-interface.md

@@ -945,29 +945,46 @@ capacity that it sees fit.
 # Sending Data {#sending}

 Once a Connection has been established, it can be used for sending data. Data
-is sent by passing a Message Object and additional parameters
-{{send-params}} to the Send Action on an established Connection:
+is always sent in association with a Message Object and additional parameters


“bound to” maybe? “Together with”?

britram · 2018-06-25T08:51:43Z

draft-ietf-taps-interface.md


 ~~~
-Connection.Send(Message, sendParameters)
+Connection.Send(Message, messageData, endOfMessage)


beating the elegance and simplicity drum again: this is correct, but it’d be nice to note here, especially, that syntactic sugar and reasonable defaults are encouraged.

I find the split between Message and messageData awkward. Also, parameters are logically separate from the message itself, so it makes sense to write this as we had it - but "endOfMessage" is just one out of many message parameters, so why have this as a "special" parameter in the send call?

britram · 2018-06-25T08:51:43Z

draft-ietf-taps-interface.md

+Each Message can be marked with Send Parameters to control how Data
+associated with the Message will be sent down to the underlying Protocol Stack 
+and transmitted. Note that these properties are per-Message, not per-Send.
+All data portions associated with a single Message share properties. For example,


I don’t like “data portions” here but I struggle to do better...

Data blocks?

britram · 2018-06-25T08:51:43Z

draft-ietf-taps-interface.md


 ~~~
-SendParameters := NewSendParameters()
-SendParameters.Add(parameter, value)
+Message := NewMessage()


So this raises the question as to why you can’t also bind an octet array to a message instance. Also platform specific sugar, but it seems to me that if you can’t bind data to a thing on send that it should probably not be called a message...

The data needs to be specific to the send action, not the Message, or else the send action becomes somewhat meaningless. You could call the Message object just MessageContext or MessageParameters or MessageMetadata, which would be a bit more accurate.

britram · 2018-06-25T08:51:43Z

draft-ietf-taps-interface.md

@@ -1185,30 +1193,41 @@ number of Messages with a single call. This allows an application to provide
 backpressure to the transport stack when it is temporarily not ready to
 receive messages.

+Receive takes an optional minIncompleteLength argument. This value indicates


in general the question of defaults should be better addressed in this draft. I think we can say that “syntactic sugar is allowed, here are the default behaviors we recommend”. Everything about partial send should be optional. This also implies we need connection-bound default send parameters...

britram · 2018-06-25T08:51:43Z

draft-ietf-taps-interface.md


 ~~~
-Connection -> Received<Message>
+Connection -> Received<Message, messageData, endOfMessage>


as with send, having a thing called a Message as well as messageData also on receive is... confusing. Renaming this to MessageContext would fix this at the expense of verbosity and slight obtuseness...

I like MessageContext

csperkins

I see where this is coming from, but I don't like the proposed API – it exposes a lot of low-level details about partial messages that I was hoping we'd get away from with the new API.

I wonder if we should regard messages as more active objects? That is, a Message is something that knows whether it's complete or not, and that can produce more data as appropriate. The API could then be to send() a Message object once, then internally the system will repeatedly call getData() on the Message object until it returns that it's complete. This makes the conceptual model easier: you just send a message. The complexity of dealing with partial messages, etc., is then pushed into the message object.

mwelzl

UPDATE: I'm much happier now after discussion with Tommy. Now it's clear that messages will only be delivered in-order, taking much of the complexity away, and we won't have messages spanning over multiple UDP packets or anything like that.

mwelzl · 2018-06-25T13:15:47Z

draft-ietf-taps-interface.md

+protocol, and provides methods to access metadata about the received data.
+The octets of data associated with this message are delivered as messageData.
+Multiple invocations of the ReceivedHandler may devlier data for the same
+Message until the endOfMessage flag is delivered. See {{receive-framing}} for 


This is THE problem I have with this whole thing - it's also the question that I raised at the end of #148 and don't see addressed. Partial messages may arrive out of order, or they may never arrive at all, if we map them on an unreliable transport. At least, applications must have a number to identify that they're now getting message fragment 3 out of 7, something like that.

mwelzl · 2018-06-25T13:15:47Z

draft-ietf-taps-interface.md


 ~~~
-Connection.Send(Message, sendParameters)
+Connection.Send(Message, messageData, endOfMessage)


I find the split between Message and messageData awkward. Also, parameters are logically separate from the message itself, so it makes sense to write this as we had it - but "endOfMessage" is just one out of many message parameters, so why have this as a "special" parameter in the send call?

mwelzl · 2018-06-25T13:15:47Z

draft-ietf-taps-interface.md

+protocol, and provides methods to access metadata about the received data.
+The octets of data associated with this message are delivered as messageData.
+Multiple invocations of the ReceivedHandler may devlier data for the same
+Message until the endOfMessage flag is delivered. See {{receive-framing}} for 


And what about partial reliability? E.g., fragments 1 and 2 out of a total of 3 arrive in time, but for the third, the time expires and the transport gives up. Shouldn't it then tell the receiving application that it will never get the third fragment, and should throw away the first two?

mwelzl · 2018-06-25T13:20:20Z

@csperkins - your idea sounds interesting, but from your description I don't understand why this reduces complexity in the API.

I think the complexity is related to ordering. If we assume that, on the receiver side, message fragments are only delivered in order, things become easier. Then we may only need an extra signal to tell the receiver that remaining fragments will never arrive.

philsbln · 2018-06-25T15:00:29Z

I like the general idea and I agree having an explicit message object really is a way out, but I also think the API in this PR exposes too many details and makes the "common" case unnecessarily complicated.

I like @csperkins Idea to move that partial send complexity into the message object.

britram · 2018-06-25T16:28:10Z

on second thought, let me +1 @csperkins on "move partial send complexity into the message object":

to send a partial message, get a message and keep writing bytes to it.
to receive a partial message, get a message that indicates that the octets read from it are not the whole message.

tfpauly · 2018-06-25T16:53:30Z

@csperkins Right, having Message objects be the active objects for sending and receiving is the other option. That's the one that we had implemented back when we were working on Minion, years ago. It certainly can work, but adds a ton of complexity to the application workflow, and reduces the ability to have meaningful back pressure and flow control.

I think that to a large degree, this PR is just writing out the underlying way that sending and receiving in the current draft works. Here's the long form of sending that works for partial and complete:

Send(messageContext, messageData, endOfMessage)

The default for endOfMessage should be true, therefore most applications will just see:

Send(messageContext, messageData)

And the the default for messageContext should be the default parameters for a new message, so most
applications (using UDP, for example) will just see:

Send(messageData)

The same is true for receive. We can give the variants of Send to be partial or not different names, but they boil down to convenience/default sets of the same thing.

The key points here are these:

The properties of a message need to be the same across multiple partial sends if there are multiple partial sends. If the application is streaming a long message, then the properties of that message apply to all of the data.
The completion handlers for Send actions that provide notions of back pressure and errors must be per data chunk sent, not per message. If I am streaming a long message, then I will likely need to handle flow control, and the completion handler for send can't just be on the whole message, it needs to be on the partial chunks.

If you move to sending data on a new Message object rather than the Connection, you would need to have back pressure on those sends too. It becomes very complicated for the application:

Connection.Send(Message) -> MessageSentHandler[May have error but can't be used for flow control]
Message.Send(bytes) -> DataSentHandler[Used for per-message flow control, which now becomes difficult to coordinate with other messages if you have a stream with HOL blocking]

Going down this road means that partial sends become front-and-center for applications dealing with the API, even when they don't want it, and adds an extra layer of indirection just to get basic bytes on the wire for a transport.

The approach in this PR ends up being a way to allow expressiveness for partial sends when they are needed, but allows applications to ignore it most of the time with defaults and write the simplest code.

britram · 2018-06-25T18:25:13Z

but allows applications to ignore it most of the time with defaults and write the simplest code.

To date we've avoided adding alternate action and event signatures to this API, leaving what gets defaulted and how up to the platform. Perhaps we should explicitly suggest/define which parts of the API should be defaulted out... I think that would go a long way to reducing the apparent complexity of this approach.

tfpauly · 2018-06-25T19:35:05Z

Right, having the default support makes things much easier. For languages that support defaults (Swift, for us), you can do the scheme like I had above. For languages like C, you can write out the common sets of calls, which all can be implemented using the most complex:

connection_partial_send(data_t data, message_context_t message, bool end_of_message)
connection_send(data_t data, message_context_t message)
connection_default_message_send(data_t data)

abrunstrom · 2018-06-25T22:53:52Z

+1 on introducing defaults. The API is beginning to get quite complex. Maybe we should explicitly split up the send section in "Basic Send Operations" and "Advanced Send Functionality" or something like that. Splitting up other relevant sections in a similar way.

tfpauly · 2018-06-26T04:33:39Z

@abrunstrom Sure, I can work tomorrow on splitting out the definition of the simple send operations, versus the more full story for partial sends. We can mention as an implementation detail that everything in the simpler approach for atomic messages can be expressed in terms of defaults in the variants that support partial sends, and leave the specific symbol bindings as a per-language choice.

chris-wood · 2018-06-26T12:01:55Z

+1 to @abrunstrom's recommendation.

britram · 2018-06-26T16:07:15Z

addressing @abrunstrom's comment here would make this ready to land IMO. I'd like to get this and #201 in soon so we can submit -01 in time, and keep working on this in person in Montreal.

tfpauly · 2018-06-26T18:39:15Z

Okay, just updated the Sending side to break out basic sends, sending with context, and partial sends. I'll do a bit more work on receiving too today, but take a look at the sending side and see if that addresses the comments there.

abrunstrom · 2018-06-26T19:59:54Z

Thanks @tfpauly for the restructuring, I like the new structure! I find it much more clear and readable.

One question on the Events in the relation to the partial sends. If I understand it correctly Sent Events relate to specific send calls whereas Expired Events relate to an entire Message. Not sure about SendError Events? Seems there it may depend on what the error is. Not sure if this is something that needs to be clarified further.

tfpauly · 2018-06-26T20:51:20Z

@abrunstrom I personally think it's simplest if the events correspond to Send events, so if you did a 2 partial sends for a single message that expired, you should get two Expired events if neither could be sent, etc.

abrunstrom · 2018-06-26T21:13:39Z

Makes sense, thanks.

tfpauly · 2018-06-26T21:38:57Z

I've added the rest of my reworked text, to bring Receives into the same style as Send (with the basic approach up front, and partial phrased as an extension later on). Thanks for all of the good feedback, I think this text is certainly clearer!

@abrunstrom I added some text about how Send events are delivered when there are multiple Sends per Message, please check.

@mwelzl I added text to explain that receiving is only in-order contiguous chunks, and clarified that you can't span a message over multiple datagrams directly, etc.

abrunstrom · 2018-06-26T22:53:11Z

Thanks @tfpauly , the added text looks good to me!

abrunstrom · 2018-06-26T22:58:42Z

draft-ietf-taps-interface.md

+check the length of the data delivered to the receive event and not assume
+it will be as long as minIncompleteLength in the case of shorter complete Messages
+or memory issues.
+


I understand that the applications should derive the length of the delivered data from the messageData. Perhaps this should be made more explicit? Before I got to the ReceivedPartial Event when reading, where the messageContext was reused for multiple events, I was thinking that the length of the data delivered may be provided to the application as part of the messageContext meta data.

Good point, I'll add an explicit comment about this.

mwelzl · 2018-06-27T07:11:56Z

draft-ietf-taps-interface.md

+allowed for a single Message. If a Message is too large to fit in the Maximum Message
+Size for the Connection, the Send will fail with a SendError event ({{send-error}}). For
+example, it is invalid to send a Message over a UDP connection that is larger than
+the available datagram sending size.

 If Send is called on a Connection which has not yet been established, an
 Initiate Action will be implicitly performed simultaneously with the Send.
 Used together with the Idempotent property (see {{send-idempotent}}), this can


A nit: I suggest to remove the "Used" at the beginning of this sentence

Agreed, removed.

mwelzl · 2018-06-27T07:20:49Z

draft-ietf-taps-interface.md

+The application can set a minIncompleteLength value to indicates the smallest partial 
+Message data size in bytes that should be delivered in response to this Receive. By default, 
+this value is infinite, which means that only complete Messages should be delivered. 
+If this value is set to some smaller value, the associated recieve event will be triggered 


typo "recieve"

Fixed! Thanks for catching.

mwelzl · 2018-06-27T07:24:03Z

This LGTM now, thanks a lot for doing this!

csperkins · 2018-06-27T09:19:36Z

@tfpauly wrote:

The key points here are these:

The properties of a message need to be the same across multiple partial sends if there are multiple partial sends. If the application is streaming a long message, then the properties of that message apply to all of the data.
The completion handlers for Send actions that provide notions of back pressure and errors must be per data chunk sent, not per message. If I am streaming a long message, then I will likely need to handle flow control, and the completion handler for send can't just be on the whole message, it needs to be on the partial chunks.

Agree with these.

If you move to sending data on a new Message object rather than the Connection,

I wasn't arguing to move the sending data onto the Message object. In my approach, the Connection would still handle sending the data, but rather than have the application provide chunks of data, I was suggesting to have the connection pull data from the message object as needed.

you would need to have back pressure on those sends too. It becomes very complicated for the application:

Don't we need that back pressure anyway? If one sends a message object from which data is pulled as needed, then I agree the connection would need to generate PartiallySent events before the final Sent event. But, we don't avoid that need by splitting the data up at the application level and making partial send requests: with a transport like TCP the connection can always block, and will need to generate a PartiallySent event, no matter whether the application has split the data into chunks before calling send or not.

It's perhaps easier for the application to change what it sends, or cancel the send, if it send chunk-by-chunk – that I agree – but otherwise I don't see a complexity saving.

tfpauly · 2018-06-27T14:31:42Z

@csperkins Regarding backpressure, you're right that we do need backpressure on all chunks that are sent. However, by allowing partial sends to be expressed in the native send action of the Connection, they automatically get the same backpressure that complete sends get. Each Sent event is one-to-one with a Send action, whether partial or complete.

tfpauly · 2018-06-27T14:32:52Z

Okay, I think based on the reviews we're in a good state to land this, and I do want to unblock merging for #201. As such, I'm going to go ahead and merge.

Rework partial sends and receives

22de20d

tfpauly added this to the ietf-01 (Montreal) milestone Jun 24, 2018

tfpauly self-assigned this Jun 24, 2018

tfpauly requested review from csperkins, britram, chris-wood, mwelzl and gorryfair Jun 24, 2018

chris-wood approved these changes Jun 25, 2018

View changes

britram approved these changes Jun 25, 2018

View changes

britram mentioned this pull request Jun 25, 2018

Send Properties: Per-send or per-message? #146

Closed

csperkins reviewed Jun 25, 2018

View changes

mwelzl requested changes Jun 25, 2018

View changes

tfpauly added 2 commits Jun 25, 2018

Review edits

6c76d56

any->the

39692f2

Restructure Sending to show Basic, then with Context, then Partial

e8dc07a

Streamline Receive into basic Receives, Partial Receives, and errors

a4e56c1

tfpauly mentioned this pull request Jun 26, 2018

Restructure Transport Properties – closes #153 and #190 #201

Merged

abrunstrom reviewed Jun 26, 2018

View changes

Specify that Data contains the length within itself

a19f828

mwelzl reviewed Jun 27, 2018

View changes

mwelzl approved these changes Jun 27, 2018

View changes

Nits from Michael

29f0e6b

tfpauly merged commit 1a7acfb into master Jun 27, 2018
2 checks passed

tfpauly mentioned this pull request Jun 27, 2018

API: Add text for sending partial messages #148

Closed

britram mentioned this pull request Jun 27, 2018

Do we need to make state storage explicit in the architecture and API? #45

Closed

britram deleted the tfp/partial-io branch Sep 26, 2018

ietf-tapswg / api-drafts Public

Rework partial sends and receives #200

Rework partial sends and receives #200

Conversation

tfpauly commented Jun 24, 2018

chris-wood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwelzl Jun 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theri commented Jun 25, 2018

britram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csperkins left a comment

mwelzl left a comment • edited

mwelzl Jun 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwelzl commented Jun 25, 2018

philsbln commented Jun 25, 2018

britram commented Jun 25, 2018

tfpauly commented Jun 25, 2018

britram commented Jun 25, 2018

tfpauly commented Jun 25, 2018

abrunstrom commented Jun 25, 2018

tfpauly commented Jun 26, 2018

chris-wood commented Jun 26, 2018

britram commented Jun 26, 2018

tfpauly commented Jun 26, 2018

abrunstrom commented Jun 26, 2018

tfpauly commented Jun 26, 2018

abrunstrom commented Jun 26, 2018

tfpauly commented Jun 26, 2018

abrunstrom commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwelzl commented Jun 27, 2018

csperkins commented Jun 27, 2018

tfpauly commented Jun 27, 2018

tfpauly commented Jun 27, 2018

mwelzl Jun 25, 2018 •

edited

mwelzl left a comment •

edited

mwelzl Jun 25, 2018 •

edited