KAFKA-5494: enable idempotence with max.in.flight.requests.per.connection > 1 by apurvam · Pull Request #3743 · apache/kafka

apurvam · 2017-08-25T19:34:08Z

Here we introduce client and broker changes to support multiple inflight requests while still guaranteeing idempotence. Two major problems to be solved:

Sequence number management on the client when there are request failures. When a batch fails, future inflight batches will also fail with OutOfOrderSequenceException. This must be handled on the client with intelligent sequence reassignment. We must also deal with the fatal failure of some batch: the future batches must get different sequence numbers when the come back.
On the broker, when we have multiple inflights, we can get duplicates of multiple old batches. With this patch, we retain the record metadata for 5 older batches.

I have added TODO(reviewers) comments for specific decisions in the code which are worth discussing.

TODO:

Add more unit tests, especially for loading different snapshot versions correctly, more client side unit tests, more broker side tests to validate that we are caching the correct number of batches (some of this is already there).
Update the system tests to check for ordering.
Run a tight loop of system tests.
Add comments about the assumptions made around the network client semantics of send/receive.
Gracefully handle changes in the producer id. In particular, we should never change the producer id once a batch has been sent, only the sequence number. This protects against duplicates and is a generalization of the previous approach to not reset producer state on retry.

apurvam · 2017-08-25T19:34:20Z

cc @ijuma @hachikuji

ijuma · 2017-08-27T08:11:13Z

Probably unintentional.

Yes. I will fix the log4j.properties as we near the end of the review process.

apurvam · 2017-08-29T18:00:27Z

Here are my todos for the patch, to help reviewers.

1. Snapshot load unit tests.
2. ProducerState tests to ensure that the correct number of batches are retained when truncation happens at head. 
3. Log tests to ensure that we handle duplicates of batches older than the last appended batch.
4. Review client tests. Do we need to add anything around batch expiry? We probably should add assertions around resetting the batch metadata on retry if we don’t do it already.
5. How does KIP-91 discussion impact this? We probably should retain the producer id and only set a new sequence if the producer id hasn’t changed. This is a generalization of the previous `isRetry` check, where we don’t change the producer id of a batch once assigned.
6. Need to upgrade the system test to enforce ordering.

hachikuji

Thanks for the patch. Left a few comments. Note that I left out comments related to expiration of batches when their ultimate outcome is unknown since we discussed this in detail offline.

hachikuji · 2017-08-28T16:13:49Z

Instead of saying we can't guarantee idempotence, maybe we can just say that idempotence is only supported for in-flight requests greater than 5. This makes it sound like it's a correctness concern, but it's not really.

hachikuji · 2017-08-28T16:16:40Z

nit: seems we don't use topicPartition.

hachikuji · 2017-08-28T16:42:43Z

nit: might help if the log message metions that this is the base sequence.

hachikuji · 2017-08-28T17:37:48Z

nit: would be nice to have consistent naming (e.g. sequences and lastAckedSequences). Also, a comment which clarifies what purpose each serves (especially for the former).

hachikuji · 2017-08-28T17:42:19Z

Seems we need to update the comment below with this check gone.

hachikuji · 2017-08-29T21:03:30Z

Not really sure this is worth checking here. Shouldn't we have ensured that duplicates weren't added to batchMetadata in the first place.

Yes. This definitely seems spurious. I was debating not including this check, and your comment confirms that I should remove it.

hachikuji · 2017-08-29T21:03:48Z

You can replace this with duplicate.headOption.

hachikuji · 2017-08-29T21:55:34Z

Hmm.. It's not clear that you're handling the case that the marker has an epoch bump below. The old sequence numbers will not be valid for the new epoch, so we should clear them as the comment below says?

As for the last offset being potentially out of sync, I'm not sure it's a problem, but it makes me a tad uncomfortable. We use this to determine when a producer should be expired (due to retention enforcement), but it may be OK if we expire a producerId when the only record left is the marker. However, we might need to be a little careful when loading the producer state from the log to make sure that producer expiration is determined consistently. Maybe we should at least rename the lastOffset method to lastDataOffset or something like that to emphasize that it does not take control records into account.

Actually, the case where the control batch has a new epoch is being handled in currentEntry.maybeUpdateEpoch. If the epoch changes we update the epoch in the ProducerIdEntry and clear all the batch metadata, which resets all the sequence numbers producer id.

I am not sure I follow your point about producer id expiration. We expire producer ids by time. However, when we are deleting segments whose retention period is elapsed, we use offsets to determine whether the producer snapshot should go. This could mean that the producer id is removed even though a control message is left. This should be fine, because that message is orphaned, and will be deleted eventually once it violates retention. I don't think this is a correctness issue at all.

Yes, I said it made me uncomfortable, not that it was necessarily a problem. I would be fine renaming the method to lastDataOffset as suggested above so that it's clear that it is not intended to include control records.

We can remove the TODO now?

hachikuji · 2017-08-29T23:17:09Z

nit: rename to dumpProducerIdSnapshot.

hachikuji · 2017-08-29T23:23:21Z

Related to my previous question, but I'm wondering how likely is it for us to hit a case where we need the additional entries after recovering the log. We would have to receive the produce requests, successfully write them to the log, and await enough time for a new snapshot to be written (which happens after rolling a segment and upon shutdown). All of that before the producer receives the responses. The cost of having this seems minor, but it's still useful to understand how much extra resilience it actually affords.

apurvam · 2017-08-30T22:06:38Z

I created a short writeup about these changes. It goes over the assumptions made, the basic solution, and also some follow up work that needs to be done to handle corner cases:

https://docs.google.com/document/d/1EBt5rDfsvpK6mAPOOWjxa9vY0hJ0s9Jx9Wpwciy0aVo/edit?usp=sharing

apurvam · 2017-09-06T22:00:16Z

Latest updates (copied from my commit message):

Batch expiry on the producer. In this case, we need mute the
partition and make sure all the inflight requests are fully resolved. If
they are successful, then all is well. If they are also expired or fail
due to OutOfOrderSequence, we need to reset the producer id.
We now deal correctly with out of order responses to in flight
requests which may happen due to leadership changes. We keep track of
all inflight batches, ordered by sequence. Whenever there is an error,
we reduce to one inflight request by ensuring only the next batch
by sequence can be drained. When we requeue a batch, we check to make
sure that the queue remains in sequence order. When a batch completes,
it is removed from the in flight list, and the next batch by sequence
changes.

Some other changes:

We never change sequence numbers unless we are certain that a batch
failed fatally on the broker (got a fatal error code), and future
batches retured OutOfOrderSequence. Before the latest commit, we would
always reset sequence numbers on retry, and have other mechanisms to
ensure that a given batch got the same sequence as before on the next
drain. This is now much tighter, and needs to be in order to correctly
handle the out of order response problem.

Todo:

Add unit tests for out of order responses.
Add unit tests new cases around batch expiry (all batches expired, all
batches succeeded, future batches failed with OutOfSequence).

hachikuji

Thanks for the updates. Mostly finished with the changes on the client side.

hachikuji · 2017-09-08T18:54:37Z

Alternatively, we could set retry to be true in the constructor when isSplitBatch is enabled? Or is there a reason we only want to set this when idempotence is in use?

Would this suggestion work?

Discussed offline, but after looking again, it seems we no longer need this at all.

hachikuji · 2017-09-08T20:10:02Z

nit: only two uses of this and one of them already has a check for transactionManager being null. Maybe we could change this to ensureQueueIsOrdered and add a null check for the other case.

hachikuji · 2017-09-08T20:46:28Z

Inadvertent string append?

hachikuji · 2017-09-08T22:45:19Z

It might be more natural to add an insertInOrder function (which executes the logic below modulo the first pollFirst) rather than inserting out of order and fixing up later.

Another thing I was wondering is whether a deque is still the right data structure given that we now require ordering. Would a priority queue give us everything we need?

I thought about this, and the deque still seems like the right one for performance reasons. We use the priority queue just for in flight batches for starters, so it is smaller. Also, we use it for a variety of purposes (like reducing to 1 in flight when there are retries). And we only need to re order the deque extremely rarely, so it is worth making that operation slightly more inefficient in the interests of keeping the performance for the common case.

Another thing is that I wanted to keep the behavior when idempotence is not enabled just the same, which is another reason I went this route.

hachikuji · 2017-09-08T22:51:38Z

Maybe worth a comment that we depend on the invariant that assigned sequence numbers for batches on the queue are all from the same producerId.

This is a good point. If the producer id changes, that means the transaction manager will stop tracking inflight batches for the previous producer id. So I updated the code to only try reordering if there are actually in flight batches by the current producer id.

Once we have batches by the current producer id, we know that that all batches by the previous producer id are no longer in play, because that is the only way to get out of the unresolved state and start draining new batches.

hachikuji · 2017-09-08T23:26:23Z

High level comment. There is a lot of per-partition state that we're tracking with separate collections. I wonder if this would be easier to manage with a PartitionState object and a single Map<TopicPartition, PartitionState>?

hachikuji · 2017-09-08T23:27:51Z

nit: maybe we could add a private static extension of PriorityQueue which sets the comparator?

hachikuji · 2017-09-08T23:30:18Z

What should the state of the builder be for this? Maybe we should verify at least that it wasn't aborted?

hachikuji · 2017-09-08T23:32:17Z

nit: this could be private? Also, there's an unneeded space after synchronized.

hachikuji · 2017-09-08T23:33:35Z

nit: maybe a better name is something like addInFlightBatch?

apurvam · 2017-09-09T00:54:05Z

One new problem we have to tackle with the new approach to duplicates. When sequnce numbers wrap around, it is possible to detect spurious duplicates. There doesn't seem to be an easy solution for this which is elegant, un less we put some bound on how old a duplicate can be. This also is worth discussing.

apurvam · 2017-09-11T17:42:48Z

retest this please

apurvam · 2017-09-11T20:25:07Z

Started system tests: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1071/

hachikuji · 2017-09-12T20:20:18Z

Mentioned previously, but I think it would be more intuitive to insert in order rather than fixing up the collection after the insertion.

Would like to hear your thoughts on my previous comment as well:

Another thing I was wondering is whether a deque is still the right data structure given that we now require ordering. Would a priority queue give us everything we need?

hachikuji · 2017-09-12T20:22:02Z

retest this please

hachikuji · 2017-09-12T20:45:39Z

Hmm.. This doesn't actually unset the producer state. Maybe reopen would be a better name?

hachikuji

A few more (smaller) comments.

hachikuji · 2017-09-12T21:25:58Z

Can't we return currentLastAckedSequence?

I chose to return -1 when there is no last ack'd sequence because it makes the logic elsewhere a bit cleaner, for instance is maybeUpdateLastAckdSequence we don't need a null check with the current setup.

That is fine, I am just questioning why we don't use the variable currentLastAckedSequence instead of calling lastAckedSequence.get(topicPartition) a second time.

ack. Will update.

hachikuji · 2017-09-12T21:26:16Z

nit: Of the micro-optimization variety, but I think it's a little nicer pattern to do a get followed by a null check rather than two hash lookups.

hachikuji · 2017-09-12T21:28:03Z

nit: annoying that we make two calls to nextBatchBySequence here. Maybe we can use a local variable?

hachikuji · 2017-09-12T21:29:22Z

Why do we not need the null check here, but we do in removeInFlightBatch? Should this be documented at least?

Good catch. Actually, we do need the null check here. The reason it exists in both places is that when we reset producer the producer id, if there is a queued batch in retry from the previous producer id, we would hit an NPE when trying to drain it after the reset. I have fixed it.

hachikuji · 2017-09-12T21:32:56Z

nit: since the argument is a partition, we could probably drop the partition prefix from the name.

hachikuji · 2017-09-12T21:40:22Z

nit: if you use a local variable for lastSequence, then you can also use it in the log message below.

Actually, the log message should contain the actual lastAckedSequence since the update is conditional: with out of order responses, it is possible that we might move the sequence backward, so the maybeUpadteLastAckedSequence guards against that.

Pardon.

In the current code base, I don't see maybeUpadteLastAckedSequence.
What's the method called now ?

hachikuji · 2017-09-12T21:43:05Z

Forgot to make this change?

hachikuji · 2017-09-12T21:57:48Z

typo: drop "the" before "whether"

hachikuji · 2017-09-12T22:09:52Z

nit: explicit returns are frowned upon in scala. Maybe we can drop the return and add an else branch? Same for duplicateOf below.

hachikuji · 2017-09-12T22:18:31Z

Yes, I said it made me uncomfortable, not that it was necessarily a problem. I would be fine renaming the method to lastDataOffset as suggested above so that it's clear that it is not intended to include control records.

apurvam · 2017-09-13T00:04:24Z

retest this please

apurvam · 2017-09-13T01:51:55Z

Some tests here are definitely leaking threads. Trying to reproduce by running tests in sequence locally.

apurvam

@hachikuji I addressed most of your comments.

I also found the cause of the thread leaks: in the new scheme, the in memory batchMetadata only caches data batches. However, if an instance of the ProducerStateManager only got a control record for a producer, we would have a ProducerIdEntry with empty batch metadata. When we go to write a snapshot, the new code would NPE, causing threads to leak in shutdown, and causing other tests to fail.

apurvam · 2017-09-13T05:28:07Z

Actually, I did try your suggestion, but it didn't quite work: for some reason spurious duplicates were being detected for some reason. So I left the old code as is.

apurvam · 2017-09-13T05:55:23Z

Good catch. Actually, we do need the null check here. The reason it exists in both places is that when we reset producer the producer id, if there is a queued batch in retry from the previous producer id, we would hit an NPE when trying to drain it after the reset. I have fixed it.

apurvam · 2017-09-13T06:00:17Z

I chose to return -1 when there is no last ack'd sequence because it makes the logic elsewhere a bit cleaner, for instance is maybeUpdateLastAckdSequence we don't need a null check with the current setup.

apurvam · 2017-09-13T06:04:28Z

good suggestion. By returning -1 from lastAckedSequence if there isn't a last acked sequence, we can reduce this to sequence - lastAckedSequence(topicPartition) == 1. Much simpler!

apurvam · 2017-09-13T06:21:06Z

However, if an instance of the ProducerStateManager only got a control record for a producer, we would have a ProducerIdEntry with empty batch metadata. When we go to write a snapshot, the new code would NPE, causing threads to leak in shutdown, and causing other tests to fail.

The specific test causing the problem was TransactionsTest.testFencingOnSend. In the current patch, we drop the batch metadata when be bump the epoch because the previous sequence numbers no longer apply and shouldn't be checked. In this test, we write a snapshot immediately after after the fence, which triggers the NPE.

hachikuji · 2017-09-13T17:36:22Z

I think we could use isNextSequence here.

hachikuji · 2017-09-13T19:53:16Z

I think we should be asserting on the result of the send futures in these test cases.

Just added the assertions for test cases in which they were missing.

hachikuji · 2017-09-13T20:34:51Z

Can we assert the offset? This ensures that the requests have not been reordered. Also, the isDone check should probably come first

Hmm. The lastAckdSequence checks already ensure that the responses are coming in the right order. We also check the sequence of the outgoing batch matches what we expect when the requests are sent, which is really what we want to check in these tests.

hachikuji · 2017-09-14T00:48:20Z

nit: here we use sequence number equality to tell when two batches match. Below in drain we use equals: https://github.com/apache/kafka/pull/3743/files#diff-e67ae6379c50ac1d9c58cefaf71346ceR540. Unless there is a good reason, maybe we should be consistent?

ack. I prefer to just use sequence number equality. Will update the drain method.

apurvam · 2017-09-14T01:04:21Z

The system tests all passed: http://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2017-09-11--001.1505200401--apurvam--KAFKA-5494-increase-max-in-flight-for-idempotent-producer--e2bd0bc/report.html

hachikuji

A few more comments. Getting close!

hachikuji · 2017-09-14T15:56:11Z

Seems like there's no longer any reason for this logic to be in sendProducerData. Maybe we may as well move it into run(now) with the rest of the other periodic transaction/idempotent checks?

hachikuji · 2017-09-14T16:28:01Z

Should we assert the failure exception?

hachikuji · 2017-09-14T16:42:40Z

nit: maybe TIMEOUT_ERROR would be better for this case since NOT_LEADER_FOR_PARTITION means the record definitely wasn't written.

By the way, doesn't this expose a weakness in our error checking? We know for sure that the write never succeeded even though we had to expire it.

Yes. I think this was brought up in the KIP-91 discussion as well: we could optimize for error codes which definitively indicate that the batch wasn't written, but we don't do that today.

This means we could get into situations where we reset the producer id where we don't strictly need to. I filed https://issues.apache.org/jira/browse/KAFKA-5897 to track this.

hachikuji · 2017-09-14T16:50:20Z

I was looking for a test case which exercised the sequence number adjustment logic, but couldn't find one. Specifically a test case which caused the sequence number of a batch to change.

Good catch. There wasn't a test, and there was a bug in that logic. Particularly, when a batch fails fatally, the future in flight batches are guaranteed to fail with an OutOfOrderSequenceException. We retry these exceptions as long as the batch in question is not the next-in-line batch. However, when we reset sequence numbers, the first future batch does become the next-in-line batch, and it is not retried when it fails with --the expected-- OutOfOrderSequenceException.

I have have the test case and the fix for the bug in an upcoming commit.

Todo: 1) Write more unit tests. 2) Handle deletion / retention / cleaning correctly.

and incremented during drain. If a batch is retried, it's sequence number is unset during the completion handler. If the first inflight batch returns an error, the next sequence to assign is reset to the last ack'd sequence + 1.

…a. This makes accesses from the cleaner threads (log cleaner, producer id expiration, etc.) safe

1. Fixed a potentioal NPE in ProducerStateManager.writeSnapshot where we could have no batch metadata in memory (for instance, if only a control batch was written), and yet write a snapshot. This was causing tests to fail because an NPE was raised during shutdown, leaking threads. 2. Addressed the remaining PR comments.

…verywhere

If a batch has a sequence, but there are no recorded inflight requests to that partition, it means that the producer id has changed, and the batch under consideration was from a previous producerId. It should always be drained (if it returns again, it will fail fatally). The code before this patch would ensure that it would _never_ drain.

…ookup

…ust the lastSeq per producer to the LogCleaner, hence simpliying the concurrency model

… expired during a transaction

… cases in SenderTest

One notable update: We didn't have a test for reassiging sequence numbers, and we had a bug in that logic. Essentially a newly reassigned batch would be fail with an out of order sequence exception because it would be the next in line batch after its sequence was adjusted. This is fixed by tracking whether the sequence of a batch has been adjusted since the last drain, in which case the out of order sequence exception for it is always retried. If it fails again, it will be fatal.

1. Move the 'reopen' flag from MemoryRecordsBuilder to ProducerBatch 2. Move the logic to resolve sequence number state to the top of the run loop. 3. More canonical scala in Log.scala.

hachikuji

LGTM. I'll merge after the builds complete.

tedyu · 2017-09-15T03:02:02Z

+
+        // If there are no inflight batches being tracked by the transaction manager, it means that the producer
+        // id must have changed and the batches being re enqueued are from the old producer id. In this case
+        // we don't try to ensure ordering amongst them. They will eventually fail with an OutOfOrderSequence,


OutOfOrderSequence -> OutOfOrderSequenceException

tedyu · 2017-09-22T18:03:46Z

+                }
+            }));
+        }
+        inflightBatchesBySequence.get(batch.topicPartition).offer(batch);


Shouldn't we add a check for the number of batches stored in the PriorityQueue ?
What if more than 5 batches are stored ?

luigiberrettini · 2020-09-24T14:25:20Z

I saw that the Sender checks for a Errors.DUPLICATE_SEQUENCE_NUMBER but I was not able to find where this error is triggered on the server side.

It seems to me that duplicate detection relies on checking if the sequence number is more than lastPersistedSeq + 1.
If this is the case:

why storing the metadata for the last batches and not just relying on the sequence number of the last message persisted in the log?
why limiting max.in.flight.requests.per.connection to a maximun value of 5 if duplicates are still detected when metadata is not found (and therefore with any number of max in flights)?

ijuma reviewed Aug 27, 2017

View reviewed changes

hachikuji reviewed Aug 29, 2017

View reviewed changes

apurvam force-pushed the KAFKA-5494-increase-max-in-flight-for-idempotent-producer branch from 1933a45 to 67230dc Compare September 6, 2017 21:59

hachikuji reviewed Sep 8, 2017

View reviewed changes

apurvam force-pushed the KAFKA-5494-increase-max-in-flight-for-idempotent-producer branch from de79b00 to b072662 Compare September 11, 2017 21:13

hachikuji reviewed Sep 12, 2017

View reviewed changes

apurvam commented Sep 13, 2017

View reviewed changes

hachikuji reviewed Sep 13, 2017

View reviewed changes

hachikuji reviewed Sep 14, 2017

View reviewed changes

Apurva Mehta added 4 commits September 14, 2017 12:48

Initial commit of client side changes with some tests

203b476

Implemented broker side changes to cache extra metadata.

546a9a7

Todo: 1) Write more unit tests. 2) Handle deletion / retention / cleaning correctly.

WIP

f4d39a8

Apurva Mehta added 17 commits September 14, 2017 12:56

Revert changes to log4j.properties

f4bf62c

Delete unnecessary trailing whitespace

c52b1f8

Let the producerIdEntry have a concurrent structure for batch metadat…

44236b8

…a. This makes accesses from the cleaner threads (log cleaner, producer id expiration, etc.) safe

Rename ProducerIdEntry.lastOffset to ProducerIdEntry.lastDataOffset e…

2ad47d4

…verywhere

Address more PR comments

66bf9fa

PR comment: return currentLastSequence instead of doing another map l…

f394747

…ookup

Remove the concurrent queue from ProducerIdEntry and instead return j…

04d9b3b

…ust the lastSeq per producer to the LogCleaner, hence simpliying the concurrency model

Address other minor comments

9c8810c

Correctly transition to a fatal error state when batches in retry are…

a617308

… expired during a transaction

Added assertions on the send futures for some of the newly added test…

78cea1c

… cases in SenderTest

check the response offsets in all the tests

9629c49

Fix checkstyle failures

9bc3ef6

Address further PR comments

27b58a4

Fix up build failures after merge

2d6b5de

apurvam force-pushed the KAFKA-5494-increase-max-in-flight-for-idempotent-producer branch from 7bab81c to 2d6b5de Compare September 14, 2017 20:00

Apurva Mehta added 2 commits September 14, 2017 13:52

Address more PR comments:

f7ad085

1. Move the 'reopen' flag from MemoryRecordsBuilder to ProducerBatch 2. Move the logic to resolve sequence number state to the top of the run loop. 3. More canonical scala in Log.scala.

Address final PR comment

b1f6530

hachikuji approved these changes Sep 14, 2017

View reviewed changes

asfgit closed this in 5d24222 Sep 14, 2017

apurvam mentioned this pull request Sep 14, 2017

KAFKA-5886: Implement KIP-91 delivery.timeout.ms #3849

Closed

tedyu reviewed Sep 15, 2017

View reviewed changes

tedyu reviewed Sep 22, 2017

View reviewed changes

ybyzek mentioned this pull request Oct 8, 2020

How to maintain message order confluentinc/kafka-tutorials#570

Closed

Nevon mentioned this pull request Dec 5, 2020

Update maxInFlightRequests to 5 tulios/kafkajs#980

Closed

Conversation

apurvam commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apurvam commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apurvam commented Aug 29, 2017

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apurvam commented Aug 30, 2017

Uh oh!

apurvam commented Sep 6, 2017

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apurvam commented Sep 9, 2017

Uh oh!

apurvam commented Aug 25, 2017 •

edited

Loading