reduce number of srds update callbacks #12118

chaoqin-li1123 · 2020-07-15T21:42:57Z

Commit Message:
Currently, in scope route discovery service, when resources are received from a management server, for each added, updated and removed scope route, a callback is fired. If several update callbacks are combined into a single update callback, less performance penalty will be incurred. This can be achieved by moving applyConfigUpdate out of the for loop and applying all the updates in a single callback. In this case, partial update won't be accepted both in sotw and delta srds. Scope key and scope name conflicts will be checked before any config update is applied.
Additional Description:
Risk Level: Low
Testing: Modify a unit test related to change.
Docs Changes: The behavior of delta srds will be changed, partial update won't be accepted.
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into srds_update

junr03 · 2020-07-16T20:00:52Z

@AndresGuedez could you take a pass at this?

AndresGuedez

Thanks! Just a few comments.

AndresGuedez · 2020-07-17T04:17:39Z

source/common/router/scoped_config_impl.cc

-    scoped_route_info_by_key_.erase(iter->second->scopeKey().hash());
-    scoped_route_info_by_name_.erase(iter);
+void ScopedConfigImpl::removeRoutingScopes(const std::vector<std::string>& scope_names) {
+  for (std::string const& scope_name : scope_names) {


nit: for consistency, I suggest using auto here as well.

AndresGuedez · 2020-07-17T04:27:31Z

source/common/router/scoped_rds.cc

-        thread_local_scoped_config->addOrUpdateRoutingScope(scoped_route_info);
-        return config;
-      });
+      updated_scopes.push_back(scoped_route_info);
      any_applied = true;
      ENVOY_LOG(debug, "srds: add/update scoped_route '{}', version: {}",


I suggest slightly changing the wording of this log message to clarify that the config has not yet been applied. e.g., "srds: queueing add/update of scoped_route ..."

This makes sense, the changes are applied after the log.

AndresGuedez · 2020-07-17T04:31:56Z

source/common/router/scoped_rds.cc

      any_applied = true;
      ENVOY_LOG(debug, "srds: add/update scoped_route '{}', version: {}",
                scoped_route_info->scopeName(), version_info);
    } catch (const EnvoyException& e) {
      exception_msgs.emplace_back(absl::StrCat("", e.what()));
    }
  }
+  if (!updated_scopes.empty()) {


Note that this changes the logic such that partial application of an SRDS config update is no longer possible. For example, if there is a duplicate scoped route, all preceding routes would have been applied prior to this change.

I actually think the new behavior is preferable; I suggest adding a test to explicitly validate this logic.

FYI @stevenzzzz

Yes, currently the srds uses SotW. A test to validate that partial update is not accepted will be good.

I think Andres is saying in delta XDS within a response handling, we could have a portion of the list applied, as the previous scope info is distributed to workers. We need to get this behavior tested.
I actually dont have a strong opinion of it, delta xDS cant send per-resource status back to management server anyway, so go ahead.

Even for a regular SOTW config update, the existing logic could partially apply config since applyConfigUpdate() is called inline for each scoped route distributed in the DiscoveryResponse.

for the SotW api, the two errors has been checked before the Delta interface is called.
Actually a good refactoring could be moving the verification code into a helper fuction. @chaoqin-li1123

Good point, thanks Xin. This only applies to delta updates.

So if the new change is applied, in delta api, partial updates is accepted in main thread when exception happens, but the partial updates are not applied to worker threads. We want the update to be fully rejected in both main thread and worker thread, right?

Maybe we can store all the incoming scoped_route_config in a vector, pass the reference of this vector to a helper function to detect any scope key or scope name conflict. If there is any conflict, no change will be applied, otherwise all changes will be applied by creating corresponding RdsRouteConfigProviderHelper and push scope_route_info into the maps.

A helper function has been added to detect key and scope name conflict.

AndresGuedez · 2020-07-17T04:32:36Z

source/common/router/scoped_rds.cc

-        thread_local_scoped_config->removeRoutingScope(scope_name);
-        return config;
-      });
+      removed_scope_names.push_back(scope_name);
      ENVOY_LOG(debug, "srds: remove scoped route '{}', version: {}", scope_name, version_info);


Same as prior comment, I suggest modifying the wording to clarify that the update has not yet been applied.

Signed-off-by: chaoqinli <chaoqinli@google.com>

stevenzzzz

looks good

stevenzzzz · 2020-07-20T12:57:36Z

source/common/router/scoped_rds.cc

+    scoped_route_map_[scoped_route_info->scopeName()] = scoped_route_info;
+    updated_scopes.push_back(scoped_route_info);
+    any_applied = true;
+    ENVOY_LOG(debug, "srds: add/update scoped_route '{}', version: {}",


out of date comment, follow Andres' suggestion here as well.

stevenzzzz · 2020-07-20T12:59:43Z

source/common/router/scoped_rds.cc

  }
  return any_applied;
-}
+} // namespace Router


sure, I will remove it.

stevenzzzz · 2020-07-20T13:00:36Z

source/common/router/scoped_rds.cc

  // Do not delete RDS config providers just yet, in case the to be deleted RDS subscriptions could
  // be reused by some to be added scopes.
-  std::list<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelperPtr>
-      to_be_removed_rds_providers = removeScopes(removed_resources, version_info);
+


extra blank lines

stevenzzzz · 2020-07-20T13:14:01Z

source/common/router/scoped_rds.cc

@@ -137,64 +138,52 @@ ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper::RdsRouteConfigProvide
 bool ScopedRdsConfigSubscription::addOrUpdateScopes(
    const std::vector<Envoy::Config::DecodedResourceRef>& resources, Init::Manager& init_manager,
    const std::string& version_info, std::vector<std::string>& exception_msgs) {
+  ASSERT(exception_msgs.empty());


no need to, this parameter is not used in this method now, remove it?

yes, and we don't need any_applied anymore if addOrUpdateScopes() doesn't do validation.

I think anyapplied is still required to give an signal to whether call the setLastConfigInfo method.

stevenzzzz · 2020-07-20T13:15:35Z

source/common/router/scoped_rds.cc

+  } catch (const EnvoyException& e) {
+    exception_msgs.emplace_back(absl::StrCat("", e.what()));
+  }
+  if (!exception_msgs.empty()) {


whynot throw in the above catch clause?

stevenzzzz · 2020-07-20T14:32:28Z

source/common/router/scoped_rds.cc

+    // if new scope have key conflict with scope that is not going to be removed
+    if (scope_name_by_hash_.find(key_fingerprint) != scope_name_by_hash_.end() &&
+        scope_name_by_hash_[key_fingerprint] != scope_name &&
+        removed_scopes.find(scope_name_by_hash_[key_fingerprint]) == removed_scopes.end()) {


use contains instead of comparing the iterator.

stevenzzzz · 2020-07-20T14:32:51Z

source/common/router/scoped_rds.cc

@@ -330,17 +346,24 @@ void ScopedRdsConfigSubscription::onConfigUpdate(
          fmt::format("scope key conflict found, first scope is '{}', second scope is '{}'",
                      scope_name_by_key_hash[key_fingerprint], scope_name));
    }
+    // if new scope have key conflict with scope that is not going to be removed
+    if (scope_name_by_hash_.find(key_fingerprint) != scope_name_by_hash_.end() &&


nit: using a iterator to avoid multiple look up.

stevenzzzz · 2020-07-20T14:47:19Z

source/common/router/scoped_rds.cc

+}
+
+Protobuf::RepeatedPtrField<std::string>
+ScopedRdsConfigSubscription::detectUpdateConflictAndCleanupRemoved(


the order is problematic here:
lets say we have {h1:scope1, h2:scope2}, and the update is like {scope1:h2, scope2:h3}
the update will be rejected as the hash of new scope1 is conflicting the original scope2.
But you can see we should not reject that, this is not introduced by your change, but rather something we can improve, could you make this improvement?

also remember deletions are done ahead of add/updates, let's add a final-state hash, apply deletions first, apply ALL the add/updates, and then count the hash conflict.

The update won't be rejected in this case.
There are 3 cases where update will be rejected:
In sotw api, when the incoming resources have key or scope name conflict.
In delta api, when the incoming resources have key or scope name conflict.
In delta api, when the incoming resources have key or scope name conflict with old scope that are not removed.

In the new procedure, in an sotw update, we mark all old scopes as removed scopes from the beginning, and conflict key and scope name with removed scopes will be ignored.

I am not following.
I think SotW and Delta form now uses the same verification logic.

There might be some confusion, but the case I was proposing is like "an incoming resource has a hash conflict with an existing scope, which will be updated by the same update but haven't yet".

right, in Delta APi, this is still a probelm. I missed your last reply, sorry.

stevenzzzz · 2020-07-20T15:58:59Z

source/common/router/scoped_rds.cc

+    // if new scope have key conflict with scope that is not going to be removed
+    if (scope_name_by_hash_.find(key_fingerprint) != scope_name_by_hash_.end() &&
+        scope_name_by_hash_[key_fingerprint] != scope_name &&
+        removed_scopes.find(scope_name_by_hash_[key_fingerprint]) == removed_scopes.end()) {


for delta API, lets say previously it's {scope1:h1}, the update is ({scope2:h1, scope1:h2},removed={})
will the update gets rejected here?

yes, it will gets rejected because scope2:h1 has key conflict with old scope with different name scope1:h1. This can be fixed easily though. Just marked all updated scopes as removed scope.

Maybe I should add a unit test to cover this corner case?

stevenzzzz · 2020-07-20T16:46:25Z

please do.

…

On Mon, Jul 20, 2020 at 12:30 PM chaoqin-li1123 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In source/common/router/scoped_rds.cc <#12118 (comment)>: > @@ -330,17 +346,24 @@ void ScopedRdsConfigSubscription::onConfigUpdate( fmt::format("scope key conflict found, first scope is '{}', second scope is '{}'", scope_name_by_key_hash[key_fingerprint], scope_name)); } + // if new scope have key conflict with scope that is not going to be removed + if (scope_name_by_hash_.find(key_fingerprint) != scope_name_by_hash_.end() && + scope_name_by_hash_[key_fingerprint] != scope_name && + removed_scopes.find(scope_name_by_hash_[key_fingerprint]) == removed_scopes.end()) { Maybe I should add a unit test to cover this corner case? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12118 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2I6TFGZPWJAZL3X5XHAQTR4RWJLANCNFSM4O3AUKCA> .

-- Xin Zhuang

Signed-off-by: chaoqinli <chaoqinli@google.com>

…s_update

AndresGuedez · 2020-07-20T17:57:13Z

source/common/router/scoped_rds.cc

+  Protobuf::RepeatedPtrField<std::string> clean_removed_resources;
+  try {
+    clean_removed_resources =
+        detectUpdateConflictAndCleanupRemoved(added_resources, removed_resources);


I suggest avoiding the use of an exception to return the conflicting resources. It complicates the error handling logic for an event that is not really "exceptional" (i.e., this function is explicitly checking for that condition).

Agree, I have changed the validation function to return normally instead of throwing an exception when detecting conflict.

AndresGuedez · 2020-07-20T18:01:31Z

test/common/router/scoped_rds_test.cc

-                ->name(),
-            "foo_routes");
+  // Scope key "x-foo-key" points to nowhere.
+  EXPECT_NE(getScopedRdsProvider(), nullptr);


I suggest the more readable (IMO): ASSERT_THAT(getScopedRdsProvider(), Not(IsNull())) instead.

Also, use ASSERT instead of EXPECT. Same below.

Sounds good.

AndresGuedez · 2020-07-20T19:22:41Z

test/common/router/scoped_rds_test.cc

-  EXPECT_EQ(getScopedRouteMap().size(), 1UL);
-  EXPECT_EQ(getScopedRouteMap().count("foo_scope"), 1);
+  // Scope key "x-foo-key" points to nowhere.
+  EXPECT_NE(getScopedRdsProvider(), nullptr);


Same as above, consider using ASSERT_THAT().

stevenzzzz · 2020-07-20T20:26:24Z

source/common/router/scoped_rds.cc

  // Do not delete RDS config providers just yet, in case the to be deleted RDS subscriptions could
  // be reused by some to be added scopes.
-  std::list<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelperPtr>
-      to_be_removed_rds_providers = removeScopes(removed_resources, version_info);
+  std::list<std::unique_ptr<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper>>


unintentional?

Signed-off-by: chaoqinli <chaoqinli@google.com>

…s_update

Signed-off-by: chaoqinli <chaoqinli@google.com>

AndresGuedez

Thanks for all the cleanup!

AndresGuedez · 2020-07-21T19:53:45Z

source/common/router/scoped_rds.cc

-  std::vector<std::string> exception_msgs;
+  std::string exception_msg;
+  Protobuf::RepeatedPtrField<std::string> clean_removed_resources;
+  detectUpdateConflictAndCleanupRemoved(added_resources, removed_resources, clean_removed_resources,


nit: it's generally preferable for readability to use a return arg as opposed to output parameters.

Agreed, I have changed clean_removed_resources to be the explicitly returned value.

Signed-off-by: chaoqinli <chaoqinli@google.com>

AndresGuedez

Thanks!

stevenzzzz

fix the nits please

stevenzzzz · 2020-07-21T21:32:58Z

source/common/router/scoped_rds.cc

      any_applied = true;
      ENVOY_LOG(debug, "srds: add/update scoped_route '{}', version: {}",
                scoped_route_info->scopeName(), version_info);
    } catch (const EnvoyException& e) {
      exception_msgs.emplace_back(absl::StrCat("", e.what()));
    }
  }
+  if (!updated_scopes.empty()) {


stevenzzzz · 2020-07-22T13:03:41Z

source/common/router/scoped_rds.cc

+    const std::vector<Envoy::Config::DecodedResourceRef>& resources,
+    const Protobuf::RepeatedPtrField<std::string>& removed_resources, std::string& exception_msg) {
+  Protobuf::RepeatedPtrField<std::string> clean_removed_resources;
+  // all the scope names to be removed or updated.


nit capitalize

stevenzzzz · 2020-07-22T13:05:42Z

source/common/router/scoped_rds.cc

+  }
+
+  absl::flat_hash_map<uint64_t, std::string> scope_name_by_hash = scope_name_by_hash_;
+  // don't check key conflict with scopes to be updated or removed


this comment is also confusing, I think we checked the conflict later in the for loops.

We only check conflict with scopes that are not updated or removed. Maybe we can change the wording a little bit to make it less confusing?

I think it's confusing as it says "we dont check conflict within the updated scopes."

How about "Don't check key conflict with old scopes to be updated or removed."?

Signed-off-by: chaoqinli <chaoqinli@google.com>

…s_update

stevenzzzz · 2020-07-22T16:33:00Z

/assign htuch

…s_update

htuch

Thanks, this seems a good idea! A few minor bits of feedback.
/wait

htuch · 2020-07-22T23:15:05Z

source/common/router/scoped_rds.cc

+  absl::flat_hash_map<uint64_t, std::string> scope_name_by_hash = scope_name_by_hash_;
+  absl::erase_if(scope_name_by_hash, [&updated_or_removed_scopes](const auto& key_name) {
+    auto const& [key, name] = key_name;
+    return (updated_or_removed_scopes.contains(name));


Nit: superfluous parens.

htuch · 2020-07-22T23:15:39Z

source/common/router/scoped_rds.cc

@@ -319,28 +340,27 @@ void ScopedRdsConfigSubscription::onConfigUpdate(
    const std::string& scope_name = scoped_route.name();
    auto scope_config_inserted = scoped_routes.try_emplace(scope_name, std::move(scoped_route));
    if (!scope_config_inserted.second) {
-      throw EnvoyException(
-          fmt::format("duplicate scoped route configuration '{}' found", scope_name));
+      exception_msg = fmt::format("duplicate scoped route configuration '{}' found", scope_name);


Why are we no longer throwing and doing error-by-value?

Because Andres think it is not a good style to throw exception in a validation function. "I suggest avoiding the use of an exception to return the conflicting resources. It complicates the error handling logic for an event that is not really "exceptional" (i.e., this function is explicitly checking for that condition)."

htuch · 2020-07-22T23:17:19Z

source/common/router/scoped_rds.cc

-  std::vector<std::string> exception_msgs;
+  std::string exception_msg;
+  Protobuf::RepeatedPtrField<std::string> clean_removed_resources =
+      detectUpdateConflictAndCleanupRemoved(added_resources, removed_resources, exception_msg);


Add comments.

htuch · 2020-07-22T23:17:56Z

source/common/router/scoped_rds.cc

+  onConfigUpdate(resources, to_remove_repeated, version_info);
+}
+
+Protobuf::RepeatedPtrField<std::string>


Can you add some comment here explaining what this does? What kind of update conflict?

Both scope name and scope key conflicts. The conflict can be between new scopes and old scopes, and it can also be between new scopes.

Signed-off-by: chaoqinli <chaoqinli@google.com>

…s_update

htuch

LGTM, thanks!

Currently, in scope route discovery service, when resources are received from a management server, for each added, updated and removed scope route, a callback is fired. If several update callbacks are combined into a single update callback, less performance penalty will be incurred. This can be achieved by moving applyConfigUpdate out of the for loop and applying all the updates in a single callback. In this case, partial update won't be accepted both in sotw and delta srds. Scope key and scope name conflicts will be checked before any config update is applied. Risk Level: Low Testing: Modify a unit test related to change. Docs Changes: The behavior of delta srds will be changed, partial update won't be accepted. Signed-off-by: chaoqinli <chaoqinli@google.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Currently, in scope route discovery service, when resources are received from a management server, for each added, updated and removed scope route, a callback is fired. If several update callbacks are combined into a single update callback, less performance penalty will be incurred. This can be achieved by moving applyConfigUpdate out of the for loop and applying all the updates in a single callback. In this case, partial update won't be accepted both in sotw and delta srds. Scope key and scope name conflicts will be checked before any config update is applied. Risk Level: Low Testing: Modify a unit test related to change. Docs Changes: The behavior of delta srds will be changed, partial update won't be accepted. Signed-off-by: chaoqinli <chaoqinli@google.com>

chaoqinli added 6 commits July 14, 2020 22:34

move callback into outer loop

90526e2

Signed-off-by: chaoqinli <chaoqinli@google.com>

move callback into outer loop

0e31325

Signed-off-by: chaoqinli <chaoqinli@google.com>

move callback into outer loop

adeeefe

Signed-off-by: chaoqinli <chaoqinli@google.com>

change unit test

db1875a

Signed-off-by: chaoqinli <chaoqinli@google.com>

clean up

6b86a52

Signed-off-by: chaoqinli <chaoqinli@google.com>

sync

4b894c4

Merge branch 'master' of https://github.com/envoyproxy/envoy into srds_update

chaoqin-li1123 changed the title ~~Srds update~~ reduce number of srds update callbacks Jul 15, 2020

sync

203370c

Merge branch 'master' of https://github.com/envoyproxy/envoy into srds_update

junr03 assigned AndresGuedez Jul 16, 2020

AndresGuedez suggested changes Jul 17, 2020

View reviewed changes

chaoqinli added 4 commits July 17, 2020 23:37

full rejection for delta srds

be714f3

Signed-off-by: chaoqinli <chaoqinli@google.com>

test comment

4aee477

Signed-off-by: chaoqinli <chaoqinli@google.com>

sync

13b2fd3

Signed-off-by: chaoqinli <chaoqinli@google.com>

format

8fe51ae

Signed-off-by: chaoqinli <chaoqinli@google.com>

chaoqin-li1123 requested review from stevenzzzz and AndresGuedez July 20, 2020 14:44

stevenzzzz reviewed Jul 20, 2020

View reviewed changes

chaoqinli added 2 commits July 20, 2020 17:16

add unit test to cover corner case

6646179

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into srd…

8ee4dfd

…s_update

AndresGuedez suggested changes Jul 20, 2020

View reviewed changes

stevenzzzz reviewed Jul 20, 2020

View reviewed changes

chaoqinli added 4 commits July 20, 2020 22:15

readability change

6135e4a

Signed-off-by: chaoqinli <chaoqinli@google.com>

readability change

7caa6aa

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into srd…

36c691d

…s_update

add unit test

16a62d3

Signed-off-by: chaoqinli <chaoqinli@google.com>

chaoqin-li1123 requested review from stevenzzzz and AndresGuedez July 21, 2020 15:09

AndresGuedez previously approved these changes Jul 21, 2020

View reviewed changes

readability

250158e

Signed-off-by: chaoqinli <chaoqinli@google.com>

chaoqin-li1123 dismissed AndresGuedez’s stale review via 250158e July 21, 2020 20:39

AndresGuedez previously approved these changes Jul 22, 2020

View reviewed changes

stevenzzzz previously approved these changes Jul 22, 2020

View reviewed changes

change comment

5ab8734

Signed-off-by: chaoqinli <chaoqinli@google.com>

chaoqin-li1123 dismissed stale reviews from stevenzzzz and AndresGuedez via 5ab8734 July 22, 2020 16:29

Merge branch 'master' of https://github.com/envoyproxy/envoy into srd…

65cef2b

…s_update

repokitteh-read-only bot assigned htuch Jul 22, 2020

Merge branch 'master' of https://github.com/envoyproxy/envoy into srd…

ec50df3

…s_update

htuch suggested changes Jul 22, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 22, 2020

add comment for key conflict detection

ab4ba97

Signed-off-by: chaoqinli <chaoqinli@google.com>

repokitteh-read-only bot removed the waiting label Jul 22, 2020

chaoqinli added 3 commits July 23, 2020 01:17

add comment for key conflict detection

6e08e2d

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into srd…

c937f08

…s_update

Merge branch 'master' of https://github.com/envoyproxy/envoy into srd…

48ee8a7

…s_update

chaoqin-li1123 requested a review from htuch July 23, 2020 23:11

htuch approved these changes Jul 24, 2020

View reviewed changes

htuch merged commit b9a41ad into envoyproxy:master Jul 24, 2020

chaoqin-li1123 deleted the srds_update branch February 23, 2021 21:18

reduce number of srds update callbacks #12118

reduce number of srds update callbacks #12118

Conversation

chaoqin-li1123 commented Jul 15, 2020 • edited Loading

junr03 commented Jul 16, 2020

AndresGuedez 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

chaoqin-li1123 Jul 17, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzzzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzzzz commented Jul 20, 2020 via email

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

AndresGuedez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresGuedez left a comment

Choose a reason for hiding this comment

stevenzzzz 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

stevenzzzz commented Jul 22, 2020

htuch 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

htuch left a comment

Choose a reason for hiding this comment

chaoqin-li1123 commented Jul 15, 2020 •

edited

Loading

chaoqin-li1123 Jul 17, 2020 •

edited

Loading