core: channelBuilder.defaulServiceConfig() and lookUpServiceConfig() #5463

dapengzhang0 · 2019-03-11T17:23:12Z

Note that the default config follows the spec that

4. If the service config is invalid, the clients should reject the received
config in its entirety and do different things depending on its state and
configuration.
	1. If it previously had selected a valid service config or no service
	config, it should continue using that previous configuration from that
	config. An empty service config is valid.
	2. If the client has not previously selected a valid service config or no
	service config, for example a new client, then:
		1. It uses the default service config, if configured and valid. Note
		that an empty default service config is a valid service config.
		2. If it does not have a default service config, it should treat the
		resolution attempt as having failed (e.g., for a polling-based
		resolver, it should retry the query after appropriate backoff). In
		other words,the channel will remain in state TRANSIENT_FAILURE until a
		valid service config is received.

…omNameResolver()

…elbuilder-service-config

zhangkun83 · 2019-03-11T22:27:00Z

core/src/main/java/io/grpc/ForwardingChannelBuilder.java

+  }
+
+  @Override
+  public T discardServiceConfigFromNameResolver() {


disableServiceConfigLookup() is shorter, and it's in line with the plan to expose this option to NameResolver so that it can skip the lookup. We also planned on logging a warning if the channel says don't lookup but the resolver did anyway. This means the channel and the resolver should work together to not look up, not just that the resolver keeps doing it while the channel will discard it.

If you go with disable, there should be enable for completeness. Or you can have lookupServiceConfig(boolean) like keepAliveWithoutCalls(boolean).

zhangkun83 · 2019-03-11T22:58:00Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+   *        If null is passed, then there will be no default service config.
+   *
+   * @throws UnsupportedOperationException If not implemented.
+   * @throws BuilderException When the given serviceConfig is invalid or the current version of grpc


We might want an exception that's specific to service config parsing, or just StatusException which is in line with parse() returning Status in the tuple.

@carl-mastrangelo @ejona86 WDYT?

Throwing IllegalArgumentException instead of a checked Exception now.

zhangkun83 · 2019-03-11T22:59:06Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

+
+        Object rawChoices = JsonParser.parse(txtRecord.substring(SERVICE_CONFIG_PREFIX.length()));
+        if (!(rawChoices instanceof List)) {
+          throw new IOException("wrong type " + rawChoices);


The convention from ServiceConfigUtil is to throw ClassCastException. We'd better follow suit.

I'm trying to minimize the change. So I would leave it unchanged in this PR.

zhangkun83 · 2019-03-11T23:16:54Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

  }

+  /**
+   * Provides a service config to the channel.


Also document the cases this default config will be used. My understanding of the spec is the default config is used only if the Channel has not seen any config from NameResolver, including the case config lookup is disabled.

zhangkun83 · 2019-03-11T23:21:25Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

+            Map<String, ?> effectiveServiceConfig =
+                config.get(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG);
+            if (effectiveServiceConfig != null
+                && effectiveServiceConfig.containsKey(DnsNameResolver.SERVICE_CONFIG_ERROR)) {


@carl-mastrangelo is implementing the error handling in the proper way. Whatever hack you put here for error-handling will be thrown away. The code as of now will just ignore the update if the config parsing failed, or the resolver didn't return a config. You may want to just keep it that way.

So can I just assume the attributes passed here is with either no config or a valid config?

Reverted the hack.

carl-mastrangelo · 2019-03-11T22:53:49Z

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

+    Map<String, Object> parsedMap = new LinkedHashMap<>();
+    for (Map.Entry<?, ?> entry : map.entrySet()) {
+      if (!(entry.getKey() instanceof String)) {
+        throw new BuilderException(null); // TODO(zdapeng): cause


ClassCastException?

ClassCastException will be wrapped. Let's wait for the decision on what Exception class should be thrown.

carl-mastrangelo · 2019-03-11T22:55:15Z

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

+  }
+
+  private static List<?> parseList(List<?> list) throws BuilderException {
+    List<Object> parsedList = new ArrayList<>();


new ArrayList<>(list.size());

carl-mastrangelo · 2019-03-11T22:57:24Z

core/src/main/java/io/grpc/ForwardingChannelBuilder.java

+  }
+
+  @Override
+  public T discardServiceConfigFromNameResolver() {


I think you should consider making this take boolean parameter.

I'm OK with lookupServiceConfig(boolean) as suggested by @zhangkun83 , let me know if any objection.

carl-mastrangelo · 2019-03-11T22:58:57Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

-        } catch (RuntimeException e) {
+        } catch (IOException | RuntimeException e) {
          logger.log(Level.WARNING, "Can't parse service Configs", e);
+          serviceConfig = Collections.singletonMap(SERVICE_CONFIG_ERROR, (Object) e);


I am in the process of modifying this, you should consider keeping the old behavior, if possible

I need to distinguish empty config (valid) and error config. In error config case, will try to use the default config; and if the default config is missing, then go to transient failure. Are you also modifying the exception handling at the original line#294?

dapengzhang0 · 2019-03-12T02:25:33Z

I reverted service config resolution error handling part, and for now assume receiving null config is always a non-error case.

ejona86

The API looks fine to me. I didn't really look at ManagedChannelImpl to verify its correctness.

ejona86 · 2019-03-12T15:53:16Z

core/src/main/java/io/grpc/ForwardingChannelBuilder.java

  }

+  @Override
+  public T defaultServiceConfig(@Nullable Map<String, ?> serviceConfig) {


This should probably be Map<String, Object>. Given the structure of serviceConfig, it's unlikely that any other value type would be useful.

The problem of Map<String, Object> might be: If the user has a Map<String, Map> object at hand, they have to cast it to Map<String, Object> first and suppress unchecked and then pass it to the API. Actually I'm not too strong about it. @carl-mastrangelo you want to debate about it?

I don't disagree, but given the service config options, what is the likelihood that they set values that are the same type? It seems they would create the object just for passing to this call, so it would also be trivial for them to use Object instead of Map<String, Object> as the value.

I could accept having it, it just seems like it would be more confusing to users without much benefit.

I found that Map<String, ?> involved less internal casting, and was easier to pass in. It also means that it isn't possible to write to the map since no type matches the wildcard, implying it is immutable. (this isn't hard and fast, but it makes it hard to call put).

How could Map<String, ?> involve less internal casting? Can you give an example?

Here is a similar case with List<Object>: https://github.com/grpc/grpc-java/blob/v1.19.x/core/src/main/java/io/grpc/internal/ServiceConfigUtil.java#L501

It's not always going to happen that way, but still it was nice to avoid that.

One other thing, when passing in this type, it's nice to pass values into Map<String, ?>, especially in unit tests where the type comes from an inlined value:

defaultServiceConfig(ImmutableMap.of("methodConfig", myTestMethodConfig));

It involves a cast to the Map<String, Object>. I think we should try it as Map<String, ?> and consider if it should be changed in API review.

ejona86 · 2019-03-12T15:56:18Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+   *
+   *        If null is passed, then there will be no default service config.
+   *
+   * @throws UnsupportedOperationException If not implemented.


We don't tend to include this exception, since our implementations implement it and it can be confusing to users.

ejona86 · 2019-03-12T15:57:07Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+  }
+
+  /**
+   * Lets the channel look up or not look up service config from the name resolver.


Specify what the default value is.

ejona86 · 2019-03-12T16:02:12Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+  /**
+   * Provides a service config to the channel.
+   *
+   * @param serviceConfig A nested map representing a Json object in the most natural way:


Don't include the table in the @param. Move that up to the general description. @param should be short, like 2 sentences max, and ideally just a sentence fragment.

ejona86 · 2019-03-12T16:05:02Z

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

+  }
+
+  @Nullable
+  private static Map<String, ?> parseMap(@Nullable Map<?, ?> map) {


This doesn't look to be parsing anything. It seems to just be making an immutable copy. Name it as such?

Renamed to checkMapEntryTypes()

ejona86 · 2019-03-12T16:06:53Z

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

+  }
+
+  @Nullable
+  private static Map<String, ?> parseMap(@Nullable Map<?, ?> map) {


Note: if you instead had parseObject, this would be much shorter as it would have less duplication between map and list. But it seems this will be deleted almost immediately, so it is fine as-is.

ejona86 · 2019-03-12T16:09:38Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

+              channelLogger.log(ChannelLogLevel.INFO, "Service config changed to null");
+            }
+
+            if (!Objects.equal(defaultServiceConfig, serviceConfig)) {


Don't bother with this condition. The equals check could be pretty expensive itself and there's virtually no chance the configs match (unless they are empty, I guess). If you want, you could check to see if they are both null (which is basically equivalent to reference equality, but more clear).

ejona86 · 2019-03-12T16:10:46Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

+            }
+
+            if (!Objects.equal(defaultServiceConfig, serviceConfig)) {
+              effectiveConfig = Attributes.newBuilder()


config.toBuilder() is more efficient than newBuilder().setAll(config) and more clear.

ejona86 · 2019-03-12T16:17:00Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

+            }
+            lastServiceConfig = defaultServiceConfig;
+          } else {
+            if (!serviceConfig.equals(lastServiceConfig)) {


Just use reference equality.

It looks this is preserving existing behavior. It can stay as-is. (Although in the future I expect it will be changed.)

zhangkun83 · 2019-03-12T22:11:45Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+  /**
+   * Provides a service config to the channel. The channel will use the default service config when
+   * the name resolver provides no service config or if the channel disables lookup service config
+   * from name resolver. The argument {@code serviceConfig} is a nested map representing a Json


add a link to lookUpServiceConfig().

zhangkun83 · 2019-03-12T22:15:09Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+  }
+
+  /**
+   * Lets the channel look up or not look up service config from the name resolver. The look-up is


The service config is not looked up from name resolver per se. It controls both whether the name resolver will look up the config, and whether the channel will use the config from name resolver.
Maybe rephrase it to: "Enable or disable service config look-up from the naming system. Enabled by default."

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

zhangkun83 · 2019-03-13T15:59:32Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

  @Nullable
  private Map<String, ?> lastServiceConfig; // used for channel tracing when value changed
+  @Nullable
+  private Map<String, ?> defaultServiceConfig;


zhangkun83 · 2019-03-13T16:42:19Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

+  // Must be mutated and read from constructor or syncContext
+  // See service config error handling spec for reference.
+  // TODO: check this value when error in service config resolution
+  private boolean selectedServiceConfigOrNoServiceConfig;


A more comprehensible name for this variable may be "waitingForServiceConfig" which defaults to true.

zhangkun83 · 2019-03-13T16:45:00Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

-          if (serviceConfig != null && !serviceConfig.equals(lastServiceConfig)) {
-            channelLogger.log(ChannelLogLevel.INFO, "Service config changed");
+          Attributes effectiveConfig = config;
+          if (!lookUpServiceConfig || serviceConfig == null) {


Maybe simplify the conditions and make it flow better:

Map<String, ?> effectiveConfig = null; // Try to use config if returned from name resolver and allowed if (serviceConfig != null) { if (lookUpServiceConfig) { effectiveConfig = serviceConfig; } else { channelLogger.log(ChannelLogLevel.INFO, "Service config from name resolver discarded by channel settings"); } } // Otherwise, try to use the default config if available if (effectiveConfig == null && defaultServiceConfig != null) { channelLogger.log(ChannelLogLevel.INFO, "Using default service config"); effectiveConfig = defaultServiceConfig; } if (effectiveConfig != lastServiceConfig) { channelLogger.log(ChannelLogLevel.INFO, "Service config changed" + (effectiveConfig == null ? " to null" : "")); lastServiceConfig = effectiveConfig; } config = config.toBuilder() .set(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG, effectiveConfig) .build(); waitingForServiceConfig = false;

zhangkun83 · 2019-03-13T16:47:44Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

+            }
+
+            if (!(defaultServiceConfig == null && serviceConfig == null)) {
+              effectiveConfig = config.toBuilder()


Even service config is already null in the attributes, it would make the implementation simpler to always update the attributes with the selected config (possibly null), because performance/allocation is not a concern for this code path. Please see my suggestion above.

That will break an existing test (Attributes.EMPTY is not the same as attributes with a key and a null value), I might need to change the test.

If it's just one test, please go ahead and update that test.

There are 6 tests, each will need introduce an ArgumentCapture, so added a check if (effectiveServiceConfig != serviceConfig) instead.

zhangkun83 · 2019-03-13T20:45:22Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

+      serviceConfigInterceptor.handleUpdate(lastServiceConfig);
+      if (retryEnabled) {
+        throttle = ServiceConfigUtil.getThrottlePolicy(lastServiceConfig);
+      }


The above 4 lines are repeated below. They need to be put in a separate method.

zhangkun83 · 2019-03-13T21:31:52Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

          nameResolverBackoffPolicy = null;

-          if (serviceConfig != null) {
+          if (lookUpServiceConfig) {


Looking at this, I think the whole service config handling logic above should also be under an if (lookUpServiceConfig), and this if-block should be grouped with that.

zhangkun83 · 2019-03-13T22:29:48Z

core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java

  }

+  @Test
+  public void disableNameResolverConfig_noDefaultConfig() throws Exception {


disableServiceConfigLookUp_

zhangkun83 · 2019-03-13T22:30:24Z

core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java

+  }
+
+  @Test
+  public void disableNameResolverConfig_withDefaultConfig() throws Exception {


zhangkun83 · 2019-03-13T22:30:46Z

core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java

+  }
+
+  @Test
+  public void noDiscardNameResolverConfig_noDefaultConfig() throws Exception {


enableServiceConfigLookUp_

zhangkun83 · 2019-03-13T22:31:01Z

core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java

+  }
+
+  @Test
+  public void noDiscardNameResolverConfig_withDefaultConfig() throws Exception {


zhangkun83 · 2019-03-13T22:34:00Z

core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java

+      LoadBalancerRegistry.getDefaultRegistry().deregister(mockLoadBalancerProvider);
+    }
+  }
+


There should also be enableServiceConfigLookUp_resolverReturnsNoConfig_withDefaultConfig and enableServiceConfigLookUp_resolverReturnsNoConfig_noDefaultConfig

zhangkun83 · 2019-03-13T23:44:30Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

-
-          waitingForServiceConfig = false;
+          } else {
+            // Try to use config if returned from name resolver and allowed


"and allowed" should be removed now.

…elbuilder-service-config

zhangkun83

LGTM

carl-mastrangelo · 2019-03-14T18:11:13Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+   *            <td>Json entry</td><td>Java Type</td>
+   *          </tr>
+   *          <tr>
+   *            <td>object</td><td>{@link Map}</td>


Map<String, ?>

Generic types, including wildcards sometimes are very inconvenient to use. I would let the users be flexible to choose whatever generic type parameter that's most convenient for them, (either the users want to minimize the times to cast and suppress unchecked, or want to minimize errorprone at compile time, or both), as long as it follows the table as a whole.

carl-mastrangelo · 2019-03-14T18:11:21Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+   *            <td>object</td><td>{@link Map}</td>
+   *          </tr>
+   *          <tr>
+   *            <td>array</td><td>{@link List}</td>


carl-mastrangelo · 2019-03-14T18:13:41Z

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

+    // Not using ImmutableMap.Builder because of extra guava dependency for Android.
+    Map<String, Object> parsedMap = new LinkedHashMap<>();
+    for (Map.Entry<?, ?> entry : map.entrySet()) {
+      if (!(entry.getKey() instanceof String)) {


This can be a checkArgument

carl-mastrangelo · 2019-03-14T18:14:55Z

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

+      } else if (value instanceof Boolean) {
+        parsedMap.put(key, value);
+      } else {
+        throw new IllegalArgumentException();


Include the type name and key

carl-mastrangelo · 2019-03-14T18:15:13Z

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

+      } else if (value instanceof Map) {
+        parsedMap.put(key, checkMapEntryTypes((Map<?, ?>) value));
+      } else if (value instanceof List) {
+        parsedMap.put(key, checkListEnttryTypes((List<?>) value));


checkListEnttryTypes is misspelled

carl-mastrangelo · 2019-03-14T18:16:02Z

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

+      return null;
+    }
+    // Not using ImmutableMap.Builder because of extra guava dependency for Android.
+    Map<String, Object> parsedMap = new LinkedHashMap<>();


new LinkedHashMap<>(map.size());

The default load factor is 0.75, it will be rehashed.

carl-mastrangelo · 2019-03-14T18:17:21Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

  private Boolean haveBackends; // a flag for doing channel tracing when flipped
-  // Must be mutated and read from syncContext
+  // Must be mutated and read from constructor or syncContext
+  // TODO: check this value when error in service config resolution


can you add my name for this todo?

dapengzhang0 · 2019-03-14T21:18:23Z

@carl-mastrangelo PTALA, hope this PR will not introduce too many merge conflicts for yours.

carl-mastrangelo · 2019-03-14T21:27:24Z

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

+    for (Map.Entry<?, ?> entry : map.entrySet()) {
+      checkArgument(
+          entry.getKey() instanceof String,
+          "The key of the entry '" + entry + "' is not of String type");


checkArgument(entry.getKey() instanceof String, "The key of the entry '%s' is not of String type", entry);

carl-mastrangelo

LGTM

dapengzhang0 · 2019-03-14T23:57:02Z

Thank you @zhangkun83 @carl-mastrangelo @ejona86 for the review!

dapengzhang0 added 4 commits March 11, 2019 10:19

core: channelBuilder.defaulServiceConfig() and discardServiceConfigFr…

21c1d97

…omNameResolver()

use wildcard for API

bb58a43

Merge branch 'master' of https://github.com/grpc/grpc-java into chann…

aa83cc7

…elbuilder-service-config

add @nullable

4eccb91

dapengzhang0 requested review from carl-mastrangelo, ejona86 and zhangkun83 March 11, 2019 22:10

ore more wildcard

1057ed9

zhangkun83 reviewed Mar 11, 2019

View reviewed changes

carl-mastrangelo reviewed Mar 11, 2019

View reviewed changes

dapengzhang0 added 3 commits March 11, 2019 17:38

refactor API

a41129e

change BuilderException to IllegalArgumentException

9e03d25

revert config resolution error handling

d4fba97

ejona86 reviewed Mar 12, 2019

View reviewed changes

dapengzhang0 changed the title ~~core: channelBuilder.defaulServiceConfig() and discardServiceConfigFromNameResolver()~~ core: channelBuilder.defaulServiceConfig() and lookUpServiceConfig() Mar 12, 2019

dapengzhang0 added 2 commits March 12, 2019 14:27

address comments

ca90224

camelCase API name and fix a bug

466c715

zhangkun83 reviewed Mar 13, 2019

View reviewed changes

dapengzhang0 added 3 commits March 13, 2019 10:24

fix small comments

9accf91

fix comment, streamline logic

40bce3f

rm dup line

b81c69d

zhangkun83 reviewed Mar 13, 2019

View reviewed changes

dapengzhang0 added 2 commits March 13, 2019 16:23

reorder logic

52d74c5

add tests

ac88cd3

zhangkun83 reviewed Mar 13, 2019

View reviewed changes

dapengzhang0 added 3 commits March 13, 2019 17:06

Merge branch 'master' of https://github.com/grpc/grpc-java into chann…

135af93

…elbuilder-service-config

fix comment

6697bf7

move effectiveAttrs declaration later

f46705b

zhangkun83 approved these changes Mar 14, 2019

View reviewed changes

carl-mastrangelo reviewed Mar 14, 2019

View reviewed changes

fix carl's comments

ceb464a

carl-mastrangelo reviewed Mar 14, 2019

View reviewed changes

minor rearrange

9198478

carl-mastrangelo approved these changes Mar 14, 2019

View reviewed changes

dapengzhang0 added 2 commits March 14, 2019 14:33

avoid string cat in checkArgument

6c75f7c

avoid one more strcat

85ea4f8

dapengzhang0 merged commit 1735adc into grpc:master Mar 14, 2019

dapengzhang0 mentioned this pull request Mar 15, 2019

Tracking API ManagedChannelBuilder.defaultServiceConfig() being experimental #5189

Open

lock bot locked as resolved and limited conversation to collaborators Jun 13, 2019

dapengzhang0 deleted the channelbuilder-service-config branch January 16, 2022 18:24

core: channelBuilder.defaulServiceConfig() and lookUpServiceConfig() #5463

core: channelBuilder.defaulServiceConfig() and lookUpServiceConfig() #5463

Uh oh!

Conversation

dapengzhang0 commented Mar 11, 2019

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!

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!

dapengzhang0 commented Mar 12, 2019

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dapengzhang0 Mar 12, 2019 • 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!

carl-mastrangelo Mar 12, 2019 • 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!

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

dapengzhang0 Mar 12, 2019 •

edited

Loading

carl-mastrangelo Mar 12, 2019 •

edited

Loading