SPEC-1168 Update transaction spec for sharded transaction recoveryToken #471

ShaneHarvey · 2019-02-07T01:03:23Z

Jira: https://jira.mongodb.org/browse/SPEC-1168

Updates transaction spec to support sharded transaction recoveryToken. This also augments the mongos session pinning rules added in SPEC-1140 because we can now send retries of commit/abort to a new mongos.

Note that to test these changes you must use the latest version of the server (at least v4.1.7-232-g473e96a) and you should decrease the transactionLifetimeLimitSeconds setting (problem described in the test README).

ShaneHarvey · 2019-02-07T01:04:58Z

source/transactions/tests/commit.json

@@ -356,7 +356,6 @@
    },
    {
      "description": "write concern error on commit",
-      "skipReason": "SERVER-37458 Mongos does not yet support writeConcern on commit",


SERVER-37458 was completed.

ShaneHarvey · 2019-02-07T01:05:26Z

source/transactions/tests/README.rst

-     (or `killAllSessionsByPattern`) command on ALL mongoses.
-
-   .. _SERVER-38335: https://jira.mongodb.org/browse/SERVER-38335
-


SERVER-38335 was completed.

ShaneHarvey · 2019-02-07T01:12:27Z

source/transactions/tests/pin-mongos.yml

+          - _id: 2
+          - _id: 3
+
+  - description: remain pinned after errors within a transaction and abort


I would like to hear some thoughts one this behavior. As described in the spec (and tested here) a session is pinned to a mongos even after a network error while running a CRUD operation within the transaction. This has the interesting side effect that a subsequent abort attempt will block until the serverSelectionTimeout (assuming the operation failed because mongos is down/unreachable).

Since it will be common to retry the entire transaction in this case it seems somewhat pointless to remain pinned for the initial abort attempt. I did not attempt to workaround this case because I wanted to keep the session pin/unpin behavior simple. Any thoughts?

Can you propose an alternative so we can evaluate? I imagine it would involve some sort of storage of the last exception in the session and then basing the abortTransaction behavior off of that?

Let's check with Aly on this as well.

I imagine it would involve some sort of storage of the last exception in the session and then basing the the abortTransaction behavior off of that?

My alternative proposal would be to unpin after transient errors within a transaction. This way a subsequent abort attempt would already be unpinned and could immediately select any available mongos. In this proposal a commit attempt that selects a new mongos would error but I wouldn't expect an app to commit after a transient error anyway.

I like that better if it works

I updated the prose to say that drivers MUST unpin a session after any transient error within a transaction. This ensures that a subsequent abort does not block and so a subsequent transaction (like withTransaction) can proceed immediately.

Note this new behavior comes with its own downside though. Due to server limitations in 4.2, an abort on a new mongos will not actually abort the in-progress transaction on the shards. The transaction will remain open and potentially block other operations. This case is possible when a mongos crashes regardless of driver behavior, but if the connection error was simply a blimp and the mongos is healthy the entire time then unpinning will cause more transactions to be left open on the server.

Personally, I prefer the updated prose. @jmikola, @EshaMaharishi, thoughts?

I'm confused, does the driver label network errors as TransientTransactionError itself?

Yes. The driver adds the TransientTransactionError error label to network errors: https://github.com/mongodb/specifications/blob/master/source/transactions/transactions.rst#transienttransactionerror

Any command error that includes the "TransientTransactionError" error label in the "errorLabels" field. Any network error or server selection error encountered running any command besides commitTransaction in a transaction. In the case of command errors, the server adds the label; in the case of network errors or server selection errors where the client receives no server reply, the client adds the label.

I see. So if the driver gets a "blip" network error and unpins from the original mongos, the driver may send abort to a different mongos, and that is the motivation for https://jira.mongodb.org/browse/SERVER-39692?

Exactly. Other similar cases are when the original mongos is restarted and looses the in memory transaction state or the original mongos is healthy but there's a network partition preventing any connections from the driver.

Ok, got it. I have no further comments.

source/transactions/transactions.rst

source/transactions/tests/README.rst

jyemin · 2019-02-11T14:24:36Z

source/transactions/tests/mongos-recovery-token.yml

+      # The first commitTransaction sees a retryable connection error due to
+      # the fail point and also fails on the server. The retry attempt on a
+      # new mongos will wait for the transaction to timeout and will fail
+      # because the transaction was aborted. Note that the retry attempt should


I think this is racy. If SDAM monitor for session0's pinned mongos happens to kick in at just the wrong time, the driver could select it.

However, in that event I think the test will still pass.

Hmm, that's a good point. The retry will succeed on the original mongos so the test will fail. Can we rely on heartbeatFrequencyMS to assume the race never happens?

What I mean is that heartbeatFrequencyMS is 500ms by default and this test runs much quicker than that (as long as the session is unpinned) so I would not expect that race condition to occur. However that might depend on how far in advance the test runner creates the client for this test. Increasing heartbeatFrequencyMS (to say 5000ms) for this test would remove the race condition.

However, I think there's another race here. When commitTransaction fails on the first attempt, the server's SDAM state is reset and a heartbeat is requested soon. This heartbeat will race with the retry attempt's server selection. To remove this race condition we can make SDAM's isMaster fail by adding failCommands: ["commitTransaction", "isMaster"].

Adding isMaster to the fail point introduces yet another race because the test runner has multiple clients connected to each server but I think this should be acceptable.

source/transactions/tests/mongos-recovery-token.yml

source/transactions/tests/pin-mongos.yml

jyemin · 2019-02-11T14:40:37Z

source/transactions/tests/pin-mongos.yml

+          - _id: 2
+          - _id: 3
+
+  - description: remain pinned after errors within a transaction and abort


Can you propose an alternative so we can evaluate? I imagine it would involve some sort of storage of the last exception in the session and then basing the abortTransaction behavior off of that?

Let's check with Aly on this as well.

source/transactions/tests/README.rst

source/transactions/tests/mongos-recovery-token.yml

source/transactions/tests/pin-mongos.yml

ShaneHarvey

Responded to many of the comments. I will get to the rest shortly.

source/transactions/transactions.rst

source/transactions/tests/pin-mongos.yml

source/transactions/tests/README.rst

ShaneHarvey · 2019-02-12T18:55:52Z

source/transactions/tests/README.rst

+outcome of a completed single-shard transaction should not block.
+Note that this test suite only includes single shard transactions.
+
+To workaorund these issues, drivers SHOULD decrease the transaction timeout


source/transactions/tests/mongos-recovery-token.yml

source/transactions/tests/pin-mongos.yml

source/transactions/tests/mongos-recovery-token.yml

ShaneHarvey · 2019-02-12T19:18:12Z

source/transactions/tests/mongos-recovery-token.yml

+data: []
+
+tests:
+  - description: commitTransaction explicit retries succeed on new mongos


Yeah it's just supposed to test that explicitly retrying commit succeeds. Renamed to simply "commitTransaction explicit retries succeed".

jmikola

I didn't have other feedback, so LGTM and will defer to others for scrutinizing the test content.

jyemin · 2019-02-19T02:11:28Z

@ShaneHarvey, there are a few unresolved comment threads that I think are in your court. Let me know if I'm mistaken.

Jeff

Unpin a session earlier now that the recoveryToken is available.

Add recoveryToken:42 to command started events

This version includes fixes for SERVER-38335 and SERVER-39187.

Intoduce object: 'testRunner' and use arguments.

source/transactions/transactions.rst

jmikola · 2019-02-19T22:54:05Z

source/transactions/transactions.rst

+^^^^^^^^^^^^^
+
+Drivers MUST unpin a ClientSession when a command within a transaction
+fails with a TransientTransactionError (including a server selection error).


Isn't "(including a server selection error)" redundant in light of the description of a TransientTransactionError? This includes network errors, server selection failures, and any other errors the server chooses to label.

Separate question: do we avoid discussing UnknownTransactionCommitResult here because the subsequent paragraph talks about unpinning as soon as a commit/abort is issued (i.e. before such a label would even be assigned)?

Fixed the "server selection error" redundancy.

Do we need to unpin after an UnknownTransactionCommitResult error? I wrote this thinking that the unpinning rule would also apply to the commitTransaction commands run as part of the commit (edit ClientSession.commitTransaction()) helper.

apply to the commitTransaction commands run as part of the commit helper.

Are you referring to ClientSession.commitTransaction() or the withTransaction() helper? I'm not sure it really matters, as the paragraph below this says:

Additionally, Drivers MUST unpin a ClientSession after running any commitTransaction or abortTransaction attempt regardless if that attempt succeeded or failed.

That surely covers both cases.

In response to this conversation with Esha, I've updated this section so that drivers remain pinned after successful commits.

EshaMaharishi

I will ask Randolph to review this as well.

EshaMaharishi · 2019-02-20T18:41:10Z

source/transactions/transactions.rst

+
+The server requires that drivers MUST send all commands for
+a single transaction to the same mongos (excluding retries of commitTransaction
+and abortTransaction) therefore this spec introduces the concept of pinning a


I don't think it's supported to send abortTransaction to a different mongos.

It may appear to work because

if the txnNumber does not match the number on the second mongos, the second mongos will return NoSuchTransaction from beginOrContinue: https://github.com/mongodb/mongo/blob/acddc7f35f0373ccb2e8fe9d45f42304b2b74f95/src/mongo/s/transaction_router.cpp#L514-L519

if the txnNumber matches the number on the second mongos, the second mongos will return NoSuchTransaction on seeing that it does not have the participant list: https://github.com/mongodb/mongo/blob/acddc7f35f0373ccb2e8fe9d45f42304b2b74f95/src/mongo/s/transaction_router.cpp#L671-L673

Yes, I've noted this server behavior elsewhere in the spec but I thought that the server will improve in the future to make aborting on a new mongos possible. The alternative is to not send abort at all (which has the same behavior as sending abort to a new mongos) or remain pinned to a potentially unreachable server and block for 30 seconds in server selection.

Is there a server ticket for this already or should I create one?

If the server started supporting aborting a transaction through a recovery mongos, there would probably be drivers changes needed (like passing the participant list back to the client, so the client can pass it to the recovery mongos).

I don't recommend drivers ever sending abortTransaction to a recovery mongos, since we have not tested that against the server and it is currently undefined in the same way using multiple mongos for transaction statements is undefined... But up to you.

I opened https://jira.mongodb.org/browse/SERVER-39692 for the server to support (or define) this behavior. Please see this conversation for why we feel drivers must unpin and send abort to a new mongos: #471 (comment)

EshaMaharishi · 2019-02-20T18:49:01Z

source/transactions/transactions.rst

-commitTransaction and abortTransaction and any retries thereof, to the
-same mongos.
+send all subsequent commands that are part of the same transaction (excluding
+retries of commitTransaction and abortTransaction) to the same mongos.


EshaMaharishi · 2019-02-20T18:52:22Z

source/transactions/transactions.rst

+^^^^^^^^^^^^^
+
+A pinned ClientSession MUST be unpinned after running the initial
+commitTransaction or abortTransaction attempt regardless if that attempt


Ditto throughout this paragraph

EshaMaharishi · 2019-02-20T18:53:35Z

source/transactions/transactions.rst

+fails on the pinned mongos, the session MUST be unpinned and any
+subsequent (automatic or explicit) retry attempt MUST perform normal mongos
+server selection. When a commitTransaction or abortTransaction
+attempt succeeds on the pinned mongos, the session MUST also be unpinned.


Why do we recommend unpinning after sending commitTransaction once?

It seems better to remain pinned, because only the original mongos can actually commit the transaction - sending commitTransaction to a recovery mongos can only be used to recover the decision.

If there's a retryable error we have to unpin to run a retry attempt. If the commit succeeds then an explicit retry can recover the previous outcome on any mongos using the recoveryToken. So optimistically unpinning the session after a commit regardless of the outcome seemed like a reasonable simplification.

However, I've now changed the spec to mandate unpinning after any transient error within a transaction so this "simplification" really no longer applies. I think I can remove this and instead say that a session must remain pinned after a successful commit (just like any other successful operation within a transaction). This would also avoid the SERVER-39349 problem that makes some of our spec tests run slower than they should.

I removed the unpin after successful commit/abort rule and updated the tests.

source/transactions/transactions.rst

EshaMaharishi · 2019-02-20T18:55:31Z

source/transactions/transactions.rst

+restarted or is not the same mongos which started the transaction.
+
+Drivers MUST treat the ``recoveryToken`` field as an opaque BSON field and
+relay it back to the server unchanged.


Should we document when the recoveryToken is returned by mongos, and that it may change (it currently does not, but we want to leave that option open for the future)?

renctan

I did not review the test cases since I don't understand the format

renctan · 2019-02-20T19:34:41Z

source/transactions/transactions.rst

+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Only included for sharded transactions and only when running a
+commitTransaction or abortTransaction command. See the


recoveryToken is only for commitTransaction. For that reason, drivers should only expect recoveryToken to exists in mongos response if ok: 1

The section below says:

When a driver runs a command within a transaction, the mongos response includes a ``recoveryToken`` field.

Do you want me to change it to say only (ok:1) responses include it? In my testing command errors also include a recoveryToken field.

It is not guaranteed to be there if the response is not ok: 1. In general, these are the cases when mongos caused the error (as opposed to errors from mongod bubbled up to mongos).

I updated to say: Every successful (``ok:1``) command response in a sharded transaction includes a ``recoveryToken`` field. ...

Should drivers also look for a recoveryToken within ok:0 responses? I could add:
Every successful (``ok:1``) command response in a sharded transaction includes a ``recoveryToken`` field. Any unsuccessful (``ok:0``) command responses in a sharded transaction MAY include a ``recoveryToken`` field. ...

For now, it is not needed for drivers to look for recoveryToken since it cannot commit the transaction anyway. The suggested text looks good to me.

Perfect, thanks.

renctan · 2019-02-20T19:37:04Z

source/transactions/transactions.rst

+~~~~~~~~~~~~~~~~~~~
+
+The ``recoveryToken`` field enables the driver to recover the outcome of a
+sharded transaction on a new (or restarted) mongos. [#]_


not any ops, but on commitTransaction commands that had an ambiguous result: like network error.

Can you expand on this? I'm not sure if you're asking for changes or clarification.

The sentence sounds like recoveryToken can be used to recover the outcome of any ops in a sharded transaction. The token is only meant to recover the outcome of a commitTransaction command.

Can you suggest a different phrasing? I'm not sure how to say "recover the outcome of a sharded transaction" more clearly.

Does the footnote below clarify this sufficiently? In 4.2, a new mongos waits for the *outcome* of the transaction but...

Perhaps just say "enables the driver to recover the outcome of a commitTransaction.

I think the current wording is fine because the sharded txn design doc uses the same language, see "Best effort to recover a transaction’s outcome via secondary router"

I just reordered it slightly to match.

renctan · 2019-02-20T19:37:31Z

source/transactions/transactions.rst

+When a driver runs a command within a transaction, the mongos response
+includes a ``recoveryToken`` field. Drivers MUST track the most recently
+received ``recoveryToken`` field and MUST append this field to any subsequent
+commitTransaction or abortTransaction commands.


only commitTransaction

Updated to only send recoveryToken with commitTransaction.

Related question: should drivers attach the recoveryToken only on retries? This way when we run the original commit against a mongos that was restarted (or a new mongos) we'll get an immediate error instead of blocking until the transaction is automatically aborted.

Currently, mongos will only use the recoveryToken only if it doesn't know who is the coordinator. So it is harmless to attach it on the non-retry cases. Not sure if this will change in the future.

I would argue that it is harmful to include it on the original attempt. When we include it on the original attempt and the mongos does not know the coordinator, the mongos will block for 60 seconds and then respond with a transient TransactionAborted error.

If we do not include it on the original attempt and the mongos does not know the coordinator, the mongos will return a transient error immediately. This behavior is essentially the same expect that it avoids blocking for 60 seconds.

I think the second case is much more desirable especially when we consider that the next operations on the session will very likely be retrying the entire transaction (as in the withTransaction API).

Given the server behavior here I'm okay with leaving the spec as is (drivers send recoveryToken on all commit attempts). I don't think the behavior of either case is ideal but at least after blocking for 60 seconds the transaction can be retried automatically by withTransaction.

I'll open a new server ticket to discuss this problem because I'm not sure drivers can workaround it.

Sounds good to me

Great, I also think the driver behavior here is correct. I think the issue is that this section from the server design doc may not have been implemented yet:

Doing so will never initiate committing the transaction. Instead, the recovery token in the request will be used to try to abort the transaction if a decision to commit has not already been made, otherwise to recover the transaction's outcome.

The 60 second waiting period should be removed once the server implements this abort logic.

Hm... I think I understand the source of the confusion for about recoveryToken for abortTransasction. The recoveryToken was meant to be used only for commit from the start. What the sentence is saying is that recovery will try to abort the transaction if decision has not been made. The current implementation is it will always try to join the active coordinator. Only if the coordinator no longer exists (because the commit finished/or was aborted) will it try to call abort (just to find out the result, if the response returned error because "transaction already committed", then we know it was committed).

Let's continue this conversation on https://jira.mongodb.org/browse/SERVER-39726?

ShaneHarvey · 2019-02-21T19:51:39Z

I think the current version of the spec is complete. There are still a few cases to follow-up on which might require driver changes (SERVER-39726, SERVER-39349, SERVER-39692) but I don't think they should hold up this review.

ShaneHarvey · 2019-02-21T20:14:08Z

Update now all the spec tests are passing in my POC here (https://github.com/ShaneHarvey/mongo-python-driver/tree/PYTHON-1684).

jmikola · 2019-02-21T21:45:08Z

source/transactions/transactions.rst

-transaction in question has already been aborted or that the pinned mongos is
+Drivers MUST unpin a ClientSession when a command within a transaction,
+including commitTransaction and abortTransaction, fails with a
+TransientTransactionError. Transient errors indicate that the transaction


I think I commented on this earlier, but commitTransaction would never fail with a TransientTransactionError. Do you need to specify UnknownTransactionCommitResult here?

The commitTransaction helper never fails with TransientTransactionError (at least never due to a network error) but the individual commands run by the helper can (I suppose this might depend on the driver's implementation?). The spec is meant to mandate that when the initial commitTransaction command attempt fails with a TransientTransactionError, drivers must unpin before running any automatic retry attempt.

Note the commitTransaction helper can fail with a TransientTransactionError when the server's command failure response includes the label. For example, when the server decides to abort the transaction instead of committing it.

The behavior is tested here: https://github.com/mongodb/specifications/pull/471/files#diff-55cfe16e56662a302c7ec014fbadadaaR173

I updated the spec to say:

Additionally, drivers MUST unpin a ClientSession when any individual commitTransaction command attempt fails with an UnknownTransactionCommitResult error label. In cases where the UnknownTransactionCommitResult causes an automatic retry attempt, drivers MUST unpin the ClientSession before performing server selection for the retry.

ShaneHarvey requested review from jmikola, mbroadst and jyemin February 7, 2019 01:03

ShaneHarvey commented Feb 7, 2019

View reviewed changes

jyemin requested changes Feb 11, 2019

View reviewed changes

jmikola reviewed Feb 11, 2019

View reviewed changes

ShaneHarvey commented Feb 12, 2019

View reviewed changes

jmikola approved these changes Feb 14, 2019

View reviewed changes

ShaneHarvey added 21 commits February 19, 2019 14:24

SPEC-1168 Update transaction spec for sharded transaction recoveryToken

86976af

Unpin a session earlier now that the recoveryToken is available.

Remove skipReason now that Mongos supports writeConcern on commit

490fa55

Test sharded recoveryToken behavior

1359f39

Add recoveryToken:42 to command started events

Test that a session remains pinned after errors within a transaction

efaf224

Add 'topology' and clientOptions.useMultipleMongoses

fa5f512

Remove outdated workaround for killAllSessions

9735eb4

Document the special failPoint operation

ec24c25

Mention that v4.1.7-232-g473e96a or later is required for sharded txns

2385f79

This version includes fixes for SERVER-38335 and SERVER-39187.

Fix format of skipReason

8be162e

Mention drivers should decrease transactionLifetimeLimitSeconds

13edd43

Fix Q&A section format

2f1bc0a

Minor comment/prose/typo fixes

7639adf

Jump through hoops to remove race condition in fails on new mongos test

ca1c750

Refactor 'failPoint' into 'targetedFailPoint'

722487f

Intoduce object: 'testRunner' and use arguments.

Drivers track most recent recoveryToken

0e38d45

Fix targetedFailPoint example in readme

a7646e3

Move topology to the top level

f17f2c5

Add topology: [replicaset, sharded] to all other tests

6f108a8

Move useMultipleMongoses outside of clientOptions

88db8c0

Drivers may assume the recoveryToken is an opaque BSON document

4bd2751

Add to changelog and bump spec version

963dc86

ShaneHarvey added 4 commits February 19, 2019 14:26

Transient errors inside transaction unpins the session

524ac2a

Rename test to be more acurate and reduce redundancy

01a808f

Initial unpin tests

ef55be7

Update modified date after rebase

34c82e6

ShaneHarvey force-pushed the SPEC-1168 branch from 778e61f to 34c82e6 Compare February 19, 2019 22:28

jmikola reviewed Feb 19, 2019

View reviewed changes

ShaneHarvey added 2 commits February 19, 2019 16:44

Add more coverage for unpinning after transient errors

f2a9bf7

Remove redundant info from unpinning prose

b02f2ad

EshaMaharishi reviewed Feb 20, 2019

View reviewed changes

renctan reviewed Feb 20, 2019

View reviewed changes

ShaneHarvey added 7 commits February 20, 2019 12:37

Drivers do not send recoveryToken with abortTransaction

1668f07

Drivers do not send recoveryToken with abortTransaction

991d23f

Use yaml anchors more aggressively

fcac728

Drivers remain pinned after successful commits and aborts

1e72dd1

Add missing comma

5f43df4

Only ok:1 responses include recoveryToken

090e444

Minor phrasing edit

162d8ec

Fix test generation script for multiple bulkWrite tests

9ddab43

jmikola reviewed Feb 21, 2019

View reviewed changes

Clarify that UnknownTransactionCommitResult unpins the session

531d346

ShaneHarvey merged commit 884d364 into mongodb:master Feb 21, 2019

		(or `killAllSessionsByPattern`) command on ALL mongoses.

		.. _SERVER-38335: https://jira.mongodb.org/browse/SERVER-38335

SPEC-1168 Update transaction spec for sharded transaction recoveryToken #471

SPEC-1168 Update transaction spec for sharded transaction recoveryToken #471

Conversation

ShaneHarvey commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyemin Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

ShaneHarvey Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Feb 19, 2019 • edited Loading

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

jyemin Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

jyemin commented Feb 19, 2019

Choose a reason for hiding this comment

ShaneHarvey Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EshaMaharishi 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

renctan 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

ShaneHarvey Feb 20, 2019 • edited Loading

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

ShaneHarvey commented Feb 21, 2019 • edited Loading

ShaneHarvey commented Feb 21, 2019

Choose a reason for hiding this comment

ShaneHarvey Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyemin Feb 11, 2019 •

edited

Loading

ShaneHarvey Feb 13, 2019 •

edited

Loading

ShaneHarvey Feb 19, 2019 •

edited

Loading

jyemin Feb 11, 2019 •

edited

Loading

ShaneHarvey Feb 20, 2019 •

edited

Loading

ShaneHarvey Feb 20, 2019 •

edited

Loading

ShaneHarvey commented Feb 21, 2019 •

edited

Loading

ShaneHarvey Feb 21, 2019 •

edited

Loading