KAFKA-9146: KIP-571 Add option to force delete active members in StreamsResetter by feyman2016 · Pull Request #8589 · apache/kafka

feyman2016 · 2020-04-30T06:38:57Z

This PR is mainly to enhance https://issues.apache.org/jira/browse/KAFKA-9146.

KafkaAdminClient#removeMembersFromConsumerGroup now support "removeAll" members in a given group
New cmdline option: --force for StreamsResetter is introduced, if --force specified when using the StreamsResetter, then all the active static/dynamic members will be removed.

Related KIP:
KIP-571: https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

mjsax · 2020-04-30T20:34:34Z

Retest this please.

Call for review @abbccdda

feyman2016 · 2020-05-01T12:43:42Z

oh, sorry I didn't run the Checkstyle and spotbugs quality checks locally, I will update shortly with these fixed

feyman2016 · 2020-05-01T14:47:11Z

Retest this please.

mjsax · 2020-05-01T23:52:46Z

@feyman2016 Only committers can trigger Jenkins retesting...

Retest this please.

mjsax · 2020-05-02T00:25:33Z

Retest this please

feyman2016 · 2020-05-02T03:20:44Z

@mjsax I see, thanks!
And call for review :)

abbccdda

Thanks for the PR, left some initial comments.

abbccdda · 2020-04-30T15:45:14Z

+        if (options.removeAll()) {
+            List<MemberIdentity> members = getMembersFromGroup(groupId);
+            findCoordinatorCall = getFindCoordinatorCall(context,
+                () -> getRemoveMembersFromGroupCall(context, members));


could we pass the members into the context?

My initial thought was to put the members in the context, but hesitated to do so because the ConsumerGroupOperationContext seems to be for generic usage. So I just refer to KafkaAdminClient#getAlterConsumerGroupOffsetsCall and make the members as a separate input param. Anyway, I'm glad to make the change if we think it's preferred to put the members in context.

Yes, I feel this is more consistent for internal calls not to do a second round of interpretation for which members set to use.

And to be clear, I'm not suggesting we have to put stuff into the context, just always passing in the intended removal list and do not depend on context.removeAll again inside internal function.

abbccdda · 2020-05-05T19:03:29Z

+        try {
+            members = describeConsumerGroups(Collections.singleton(groupId)).describedGroups().get(groupId).get().members();
+        } catch (Throwable ex) {
+            System.out.println("Encounter exception when trying to get members from group: " + groupId);


Remove print statements

abbccdda · 2020-05-05T19:04:30Z

+            members = describeConsumerGroups(Collections.singleton(groupId)).describedGroups().get(groupId).get().members();
+        } catch (Throwable ex) {
+            System.out.println("Encounter exception when trying to get members from group: " + groupId);
+            ex.printStackTrace();


Curious why we are still continuing in this case, as the member lookup already fails.

Thanks, will fix this .

abbccdda · 2020-05-05T19:07:17Z

@@ -32,12 +32,23 @@
 public class RemoveMembersFromConsumerGroupOptions extends AbstractOptions<RemoveMembersFromConsumerGroupOptions> {

    private Set<MemberToRemove> members;


Could we just make members to be Optional<Set<MemberToRemove>> so that we don't need a separate removeAll parameter?

Sure. Taking a step further, can we just keep the the type Set<MemberToRemove> for members unchanged and treat it as removeAll if the members is empty set?

abbccdda · 2020-05-05T19:12:50Z

+        }
+
+        List<MemberIdentity> memberToRemove = new ArrayList<>();
+        for (MemberDescription member: members) {


style error here.

I would recommend doing a self style check like:
./gradlew checkstyleMain checkstyleTest spotbugsMain spotbugsTest spotbugsScoverage compileTestJava otherwise we still need to fix those failures after we do jenkins build.

Thanks for the advice, will fix it in the next commit.

I reran the self style check, but didn't capture any error. I assume the error would be the missed final in for loop, updated.

feyman2016 · 2020-05-06T01:55:23Z

@abbccdda Thanks a lot for the review, will update soon.

feyman2016 · 2020-05-07T03:29:40Z

@abbccdda Hey, updated based on comments, and also left some comments there, thanks.

feyman2016 · 2020-05-09T10:03:20Z

Call for retest and review, thanks!

abbccdda · 2020-05-13T06:21:20Z

@feyman2016 Sure thing!

abbccdda

Thanks for the update. I could see we are still having a couple of places with style violation, did you properly configure and run checkstyle locally?

abbccdda · 2020-05-13T06:23:08Z

+        }
+
+        List<MemberIdentity> memberToRemove = new ArrayList<>();
+        for (final MemberDescription member: members) {


nit: space before :

abbccdda · 2020-05-13T15:04:03Z

+        if (options.removeAll()) {
+            List<MemberIdentity> members = getMembersFromGroup(groupId);
+            findCoordinatorCall = getFindCoordinatorCall(context,
+                () -> getRemoveMembersFromGroupCall(context, members));


Yes, I feel this is more consistent for internal calls not to do a second round of interpretation for which members set to use.

abbccdda · 2020-05-13T15:07:20Z

+    public boolean removeAll() {
+        return members.isEmpty();
+    }
+


nit: remove extra line

abbccdda · 2020-05-13T15:11:24Z

+                () -> getRemoveMembersFromGroupCall(context, members));
+        } else {
+            findCoordinatorCall = getFindCoordinatorCall(context,
+                () -> getRemoveMembersFromGroupCall(context, new ArrayList<>()));


Should be Collection.emptyList()

abbccdda · 2020-05-13T15:13:24Z

                // If coordinator changed since we fetched it, retry
                if (ConsumerGroupOperationContext.hasCoordinatorMoved(response)) {
-                    Call call = getRemoveMembersFromGroupCall(context);
+                    Call call = getRemoveMembersFromGroupCall(context, allMembers);


Why do we blindly put allMembers? I believe we base on context to interpret, but like discussed earlier, this is easy to make mistake, we should rely on one source for members.

Fixed, now we explicitly pass in the members to be deleted to the private getRemoveMembersFromGroupCall

abbccdda · 2020-05-13T15:23:22Z

        Utils.closeQuietly(producer, "producer")
      }
+
+      val EMPTY_GROUP_INSTANCE_ID = ""


I don't think we really need this struct, could we just put null in groupInstanceSet?

I feel like this is more informative, so didn't update it, but yeah, I can update if we really not prefer this~

Fair enough

abbccdda · 2020-05-13T15:23:44Z

+              try {
+                while (true) {
+                  consumer.poll(JDuration.ofSeconds(5))
+                  if ( !consumer.assignment.isEmpty && latch.getCount > 0L)


abbccdda · 2020-05-13T15:24:16Z

+                  consumer.commitSync()
+                }
+              } catch {
+                case _: InterruptException => // Suppress the output to stderr


Why do we suppress here?

Didn't change the exception handling logic here, just extract the Thread creation logic to reuse~

abbccdda · 2020-05-13T15:25:51Z

          assertFalse(testGroupDescription.isSimpleConsumerGroup)
+          assertEquals(consumerSet.size -1, testGroupDescription.members().size())
+
+          // Delete all active members remained (a static member + a dynamic member)


remained -> remaining

abbccdda · 2020-05-13T15:29:16Z

            .withRequiredArg()
            .ofType(String.class)
            .describedAs("file name");
+        forceOption = optionParser.accepts("force", "Force remove members when long session time out has been configured, " +


Do we also want to edit the usage info on top to mention the force delete option?

I think so, updated

feyman2016 · 2020-05-15T06:32:14Z

@abbccdda Sorry for causing too much trouble about style....will make sure style check correctly executed before requesting for review next time.

abbccdda · 2020-05-16T05:30:05Z

@feyman2016 no worry! It's just for first time :)

feyman2016 · 2020-05-19T09:51:54Z

@abbccdda I'm fixing the style error, and I found that it showed success if I run the below cmd locally :
./gradlew checkstyleMain checkstyleTest spotbugsMain spotbugsTest spotbugsScoverage compileTestJava
I'm didn't find the reason after some investigating, but the style check cmdline can capture style error if I cherry-pick the first commit of this PR to another branch, guessing that it might be the second commit: merge trunk somehow made the style check doesn't work...
So would it break our convention if I revert to the first commit:Add option to force delete active members in StreamsResetter and fix style error from there ? This may change the commit history of this PR.
I'm asking because I noticed :"Please address feedback via additional commits instead of amending existing commits." on https://cwiki.apache.org/confluence/display/KAFKA/Contributing+Code+Changes#ContributingCodeChanges-PullRequest
Thanks!

abbccdda · 2020-05-19T16:03:34Z

@feyman2016 Thanks for the context. I don't worry too much for comment vanish if you change the commit history, as they would not be gone but just show as outdated on github. Just do whatever you feel makes sense.

Also just a reminder that we are one week away from the feature freeze, so let's try to ramp up and get this into 2.6.

feyman2016 · 2020-05-20T07:44:46Z

@abbccdda Thanks, will try my best to get this into 2.6

feyman2016 · 2020-05-21T11:12:04Z

@abbccdda Updated, call for review, thanks!

abbccdda

Thanks for the update @feyman2016 ! Got some more comments, but I think we are getting there :)

abbccdda · 2020-05-21T15:51:55Z

+        Collection<MemberDescription> members;
+        try {
+            members = describeConsumerGroups(Collections.singleton(groupId)).describedGroups().get(groupId).get().members();
+        } catch (Throwable ex) {


I think we should catch Exception here:
https://stackoverflow.com/questions/2274102/difference-between-using-throwable-and-exception-in-a-try-catch

Make sense, fixed~

abbccdda · 2020-05-21T15:56:31Z

+        for (final MemberDescription member : members) {
+            if (member.groupInstanceId().isPresent()) {
+                memberToRemove.add(new MemberIdentity().setGroupInstanceId(member.groupInstanceId().get())
+                );


This indentation is a bit weird, let's just merge L3625-3626

abbccdda · 2020-05-21T16:00:08Z


        ConsumerGroupOperationContext<Map<MemberIdentity, Errors>, RemoveMembersFromConsumerGroupOptions> context =
-            new ConsumerGroupOperationContext<>(groupId, options, deadline, future);
+                new ConsumerGroupOperationContext<>(groupId, options, deadline, future);


Let's get back the original indentation.

abbccdda · 2020-05-21T16:02:44Z

-                return new LeaveGroupRequest.Builder(context.groupId(),
-                                                     context.options().members().stream().map(
-                                                         MemberToRemove::toMemberIdentity).collect(Collectors.toList()));
+                    return new LeaveGroupRequest.Builder(context.groupId(),


nit: we could merge L3666-3667

abbccdda · 2020-05-21T16:03:26Z


-    private Call getRemoveMembersFromGroupCall(ConsumerGroupOperationContext<Map<MemberIdentity, Errors>, RemoveMembersFromConsumerGroupOptions> context) {
+    private Call getRemoveMembersFromGroupCall(ConsumerGroupOperationContext<Map<MemberIdentity, Errors>,
+            RemoveMembersFromConsumerGroupOptions> context, List<MemberIdentity> allMembers) {


nit: we could name it members now

Yeah, fixed

abbccdda · 2020-05-21T16:33:50Z

+            consumerThread.join()
+        }
+      }
+      }finally {


nit: format
I'm pretty surprised this wasn't caught in my previous template. Let me check how to cover this in style test as well.

Thanks, but I wonder what does the template refer to here?

abbccdda · 2020-05-21T16:35:08Z

+    public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
+        appID = testId + "-with-force-option";
+        streamsConfig.put(StreamsConfig.APPLICATION_ID_CONFIG, appID);
+        streamsConfig.put(ConsumerConfig.SESSION_TIMEOUT_MS_CONFIG, "" + STREAMS_CONSUMER_TIMEOUT * 100);


What does "" + mean?

Without the "" + to convert the value to String, we will get exception like: it is because STREAMS_CONSUMER_TIMEOUT = 2000L, ""+ is widely used in this test, just follow it here without any change to not enlarge the scope of this PR, I can help to create a Jira to enhance it if we think this workaround is not quite intuitive~

org.apache.kafka.common.config.ConfigException: Invalid value 200000 for configuration session.timeout.ms: Expected value to be a 32-bit integer, but it was a java.lang.Long at org.apache.kafka.common.config.ConfigDef.parseType(ConfigDef.java:672) at org.apache.kafka.common.config.ConfigDef.parseValue(ConfigDef.java:474) at org.apache.kafka.common.config.ConfigDef.parse(ConfigDef.java:467) at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:108) at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:129) at org.apache.kafka.clients.consumer.ConsumerConfig.<init>(ConsumerConfig.java:606) at org.apache.kafka.clients.consumer.KafkaConsumer.<init>(KafkaConsumer.java:630) at org.apache.kafka.streams.processor.internals.DefaultKafkaClientSupplier.getRestoreConsumer(DefaultKafkaClientSupplier.java:56) at org.apache.kafka.streams.processor.internals.StreamThread.create(StreamThread.java:313) at org.apache.kafka.streams.KafkaStreams.<init>(KafkaStreams.java:766) at org.apache.kafka.streams.KafkaStreams.<init>(KafkaStreams.java:652) at org.apache.kafka.streams.KafkaStreams.<init>(KafkaStreams.java:562) at org.apache.kafka.streams.integration.AbstractResetIntegrationTest.testResetWhenLongSessionTimeoutConfiguredWithForceOption(AbstractResetIntegrationTest.java:270) at org.apache.kafka.streams.integration.ResetIntegrationTest.testResetWhenLongSessionTimeoutConfiguredWithForceOption(ResetIntegrationTest.java:77)

I see, this is indeed weird, please file a JIRA so that we could clean in a follow-up PR if others feel the same way.

Created https://issues.apache.org/jira/browse/KAFKA-10035 for tracking, thanks!

abbccdda · 2020-05-21T16:36:25Z


-    private void cleanGlobal(final boolean withIntermediateTopics,
+    private int tryCleanGlobal(final boolean withIntermediateTopics,
                             final String resetScenario,


nit: parameters are not aligned.

abbccdda · 2020-05-21T16:37:52Z

        cleanUpConfig.put(ConsumerConfig.SESSION_TIMEOUT_MS_CONFIG, "" + CLEANUP_CONSUMER_TIMEOUT);

        final int exitCode = new StreamsResetter().run(parameters, cleanUpConfig);
+        return exitCode;


Like said earlier, I think we could just return
return new StreamsResetter().run(parameters, cleanUpConfig) == 0

abbccdda · 2020-05-21T16:38:16Z

    }

-    private void cleanGlobal(final boolean withIntermediateTopics,
+    private int tryCleanGlobal(final boolean withIntermediateTopics,


We could add meta comment for the return value here, and instead of returning an exit code, I feel a boolean is suffice to indicate whether the clean operation was successful or not.

Indeed, updated as suggested

mjsax · 2020-05-27T03:13:55Z

     */
    public KafkaFuture<Void> memberResult(MemberToRemove member) {
+        if (removeAll()) {
+            throw new IllegalArgumentException("The method: memberResult is not applicable in 'removeAll' mode");


Why that? I understand that we expect that users don't know the memberId if the so a "remove all"; however, I don't see why we need to disallow this call? Can you elaborate?

Since in the removeAll scenario, we don't save the members to be deleted in RemoveMembersFromConsumerGroupResult, so I think calling memberResult doesn't seem applicative.

I see. Makes sense.

mjsax · 2020-05-27T03:15:16Z

            MetadataResponse.AUTHORIZED_OPERATIONS_OMITTED);
    }

+    private static DescribeGroupsResponseData prepareDescribeGroupsResponseData(String groupId, List<String> groupInstances,


Nit: formatting

private static DescribeGroupsResponseData prepareDescribeGroupsResponseData(String groupId, List<String> groupInstances, List<TopicPartition> topicPartitions) {

mjsax · 2020-05-27T03:21:36Z

-        new NewTopic(testTopicName, testNumPartitions, 1.toShort))).all().get()
-      waitForTopics(client, List(testTopicName), List())
+
+      client.createTopics(util.Arrays.asList(new NewTopic(testTopicName, testNumPartitions, 1.toShort),


nit: formatting: move new NewTopic(...) to next line

mjsax

Overall, LGTM. Couple of comments/questions.

Co-authored-by: Matthias J. Sax <mjsax@apache.org>

mjsax · 2020-05-27T18:09:55Z

Retest this please.

mjsax · 2020-05-27T18:10:10Z

Retest this please.

feyman2016 · 2020-05-27T18:26:17Z

@mjsax Thanks a lot for the review and tests triggering!

mjsax · 2020-05-27T21:09:07Z

Java 8:

org.apache.kafka.streams.integration.RegexSourceIntegrationTest.testRegexRecordsAreProcessedAfterReassignment
org.apache.kafka.streams.integration.RegexSourceIntegrationTest.testRegexMatchesTopicsAWhenCreated

Java 11:

org.apache.kafka.connect.integration.InternalTopicsIntegrationTest.testCreateInternalTopicsWithDefaultSettings
org.apache.kafka.streams.integration.EosBetaUpgradeIntegrationTest.shouldUpgradeFromEosAlphaToEosBeta[true]

Java 14:

org.apache.kafka.streams.integration.EosBetaUpgradeIntegrationTest.shouldUpgradeFromEosAlphaToEosBeta[true]

mjsax · 2020-05-27T21:09:15Z

Retest this please.

mjsax · 2020-05-27T21:09:41Z

Retest this please.

mjsax · 2020-05-27T21:13:31Z

Retest this please.

mjsax · 2020-05-27T21:14:33Z

Retest this please.

mjsax · 2020-05-27T21:15:23Z

Jenkins does not cooperate... will try again later

mjsax · 2020-05-27T21:35:42Z

Retest this please.

feyman2016 · 2020-05-28T00:53:44Z

Finally, jenkins cooperated :)

mjsax · 2020-05-28T01:41:15Z

Thanks for the KIP and PR @feyman2016!

abbccdda · 2020-05-28T01:42:36Z

Great work, @feyman2016 @mjsax !

feyman2016 · 2020-05-28T11:55:17Z

@abbccdda @mjsax Thanks a lot for your kindly review and help! Are you guys using some formatting tool, I asked this because this PR had too many formatting issues, would be good if next time I could effectively avoid them, thanks!

feyman2016 · 2020-05-28T13:01:08Z

FYI, since we took a slightly different implementation(leveraging the empty members rather than introducing a new field to imply the removeAll scenario), I updated the KIP-571 accordingly to keep them consistent.

mjsax · 2020-05-28T21:24:49Z

Are you guys using some formatting tool

I use IntelliJ that does have some auto-formatting. Use it basically with default settings. In doubt, disable auto-formatting to avoid unnecessary reformatting. -- Some things are also a little bit of "personal taste"...

I updated the KIP-571 accordingly to keep them consistent.

Thanks! Can you send a follow up email to the voting thread explaining the change? To make sure nobody has concerns about it.

feyman2016 · 2020-05-30T04:30:43Z

@mjsax Sure, updated in the voting thread of KIP-571

* 'trunk' of github.com:apache/kafka: (36 commits) Remove redundant `containsKey` call in KafkaProducer (apache#8761) KAFKA-9494; Include additional metadata information in DescribeConfig response (KIP-569) (apache#8723) KAFKA-10061; Fix flaky `ReassignPartitionsIntegrationTest.testCancellation` (apache#8749) KAFKA-9130; KIP-518 Allow listing consumer groups per state (apache#8238) KAFKA-9501: convert between active and standby without closing stores (apache#8248) KAFKA-10056; Ensure consumer metadata contains new topics on subscription change (apache#8739) MINOR: Log the reason for coordinator discovery failure (apache#8747) KAFKA-10029; Don't update completedReceives when channels are closed to avoid ConcurrentModificationException (apache#8705) MINOR: remove unnecessary timeout for admin request (apache#8738) MINOR: Relax Percentiles test (apache#8748) MINOR: regression test for task assignor config (apache#8743) MINOR: Update documentation.html to refer to 2.6 (apache#8745) MINOR: Update documentation.html to refer to 2.5 (apache#8744) KAFKA-9673: Filter and Conditional SMTs (apache#8699) KAFKA-9971: Error Reporting in Sink Connectors (KIP-610) (apache#8720) KAFKA-10052: Harden assertion of topic settings in Connect integration tests (apache#8735) MINOR: Slight MetadataCache tweaks to avoid unnecessary work (apache#8728) KAFKA-9802; Increase transaction timeout in system tests to reduce flakiness (apache#8736) KAFKA-10050: kafka_log4j_appender.py fixed for JDK11 (apache#8731) KAFKA-9146: Add option to force delete active members in StreamsResetter (apache#8589) ... # Conflicts: # core/src/main/scala/kafka/log/Log.scala

feyman2016 added 3 commits April 30, 2020 14:31

Add option to force delete active members in StreamsResetter

8f00c6f

merge trunk

617dcf6

update

e02c3c6

feyman2016 changed the title ~~KAFKA-9146: Add option to force delete active members in StreamsResetter~~ KAFKA-9146: KIP-571 Add option to force delete active members in StreamsResetter Apr 30, 2020

mjsax added streams tools labels Apr 30, 2020

fix checkstyle violation

c6b4ef3

mjsax mentioned this pull request May 1, 2020

KAFKA-9146 [WIP]: Add option to force delete active members in StreamsResetter #8020

Closed

3 tasks

abbccdda reviewed May 5, 2020

View reviewed changes

fix based on comments

6a845f3

abbccdda reviewed May 13, 2020

View reviewed changes

feyman2016 added 2 commits May 21, 2020 16:03

update based on comments

6b15ded

fix comment

d43ce29

abbccdda reviewed May 21, 2020

View reviewed changes

mjsax reviewed May 27, 2020

View reviewed changes

feyman2016 and others added 6 commits May 27, 2020 15:26

Update core/src/main/scala/kafka/tools/StreamsResetter.java

6dedd1c

Co-authored-by: Matthias J. Sax <mjsax@apache.org>

Update core/src/main/scala/kafka/tools/StreamsResetter.java

f015c22

Co-authored-by: Matthias J. Sax <mjsax@apache.org>

Update core/src/main/scala/kafka/tools/StreamsResetter.java

f92a3f6

Co-authored-by: Matthias J. Sax <mjsax@apache.org>

Update core/src/main/scala/kafka/tools/StreamsResetter.java

87346c0

Co-authored-by: Matthias J. Sax <mjsax@apache.org>

fix comments

174ba7e

refactor IntegrationTestUtils

5329315

mjsax merged commit 90045f6 into apache:trunk May 28, 2020

mjsax added the kip Requires or implements a KIP label Jun 12, 2020

		@@ -32,12 +32,23 @@
		public class RemoveMembersFromConsumerGroupOptions extends AbstractOptions<RemoveMembersFromConsumerGroupOptions> {

		private Set<MemberToRemove> members;

Conversation

feyman2016 commented Apr 30, 2020

Committer Checklist (excluded from commit message)

Uh oh!

mjsax commented Apr 30, 2020

Uh oh!

feyman2016 commented May 1, 2020

Uh oh!

feyman2016 commented May 1, 2020

Uh oh!

mjsax commented May 1, 2020

Uh oh!

mjsax commented May 2, 2020

Uh oh!

feyman2016 commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abbccdda 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!

feyman2016 May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

feyman2016 May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

feyman2016 May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feyman2016 commented May 6, 2020

Uh oh!

feyman2016 commented May 7, 2020

Uh oh!

feyman2016 commented May 9, 2020

Uh oh!

abbccdda commented May 13, 2020

Uh oh!

abbccdda 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

feyman2016 commented May 2, 2020 •

edited

Loading

feyman2016 May 6, 2020 •

edited

Loading

feyman2016 May 6, 2020 •

edited

Loading

feyman2016 May 6, 2020 •

edited

Loading

feyman2016 commented May 19, 2020 •

edited

Loading