xds: add ConfigSelector to support RouteAction timeouts #3991

dfawley · 2020-10-26T22:34:49Z

This change is

menghanl

Reviewed 7 of 9 files at r1.
Reviewable status: 7 of 9 files reviewed, 7 unresolved discussions (waiting on @dfawley and @menghanl)

clientconn.go, line 648 at r1 (raw file):

			cc.applyServiceConfigAndBalancer(sc, s.Addresses)
			if configSelector := iresolver.GetConfigSelector(s); configSelector != nil {
				fmt.Println("received non-nil config selector yay")

s/yay/lol

service_config.go, line 108 at r1 (raw file):

}

type retryPolicy = internalserviceconfig.RetryPolicy

Why do we still need this type?
Just use internalserviceconfig.RetryPolicy?

stream.go, line 177 at r1 (raw file):

	rpcConfig := cc.safeConfigSelector.SelectConfig(iresolver.RPCInfo{Context: ctx, Method: method})
	if rpcConfig == nil {
		rpcConfig = &iresolver.RPCConfig{Context: ctx}

Can this save one allocation

var mc serviceconfig.MethodConfig
if rpcConfig != nil {
  ctx = rpcConfig.Context
  mc = rpcConfig.MethodConfig

  // And rpcConfig.OnCommitted???
}

internal/resolver/config_selector.go, line 44 at r1 (raw file):

// RPCConfig describes the configuration to use for each RPC.
type RPCConfig struct {
	Context      context.Context            // passes info to LB Policy Picker

And talk about timeout? It could be overlooked.

internal/resolver/config_selector.go, line 46 at r1 (raw file):

	Context      context.Context            // passes info to LB Policy Picker
	MethodConfig serviceconfig.MethodConfig // configuration to use for this RPC
	OnCommitted  func()                     // Called when the RPC has been committed (retries no longer possible)

This is never called.

internal/resolver/safe_config_selector.go, line 31 at r1 (raw file):

// implementations such that previous values are guaranteed to not be in use
// when UpdateConfigSelector returns.
type SafeConfigSelector struct {

This is only used by the ClientConn, right?

Is it like pick-wrapper (a helper struct for ConfigSelector)? Move it to the grpc package?

internal/resolver/safe_config_selector.go, line 66 at r1 (raw file):

		ccsPtr = ccsPtr2
	}
	defer ccs.wg.Done()

I don't remember all the details now...
Should this be done in OnCommitted()?

dfawley

Reviewable status: 7 of 9 files reviewed, 7 unresolved discussions (waiting on @menghanl)

clientconn.go, line 648 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

s/yay/lol

Argh, I swear I deleted all the debugging stuff. Removed.

service_config.go, line 108 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Why do we still need this type?
Just use internalserviceconfig.RetryPolicy?

Good point. Done.

stream.go, line 177 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Can this save one allocation

var mc serviceconfig.MethodConfig
if rpcConfig != nil {
  ctx = rpcConfig.Context
  mc = rpcConfig.MethodConfig

  // And rpcConfig.OnCommitted???
}

The compiler should be able to optimize that, but why take chances? Done. Also implemented OnCommitted.

internal/resolver/config_selector.go, line 44 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

And talk about timeout? It could be overlooked.

Done.

internal/resolver/config_selector.go, line 46 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

This is never called.

Done.

internal/resolver/safe_config_selector.go, line 31 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

This is only used by the ClientConn, right?

Is it like pick-wrapper (a helper struct for ConfigSelector)? Move it to the grpc package?

Yes, but I felt like it was more of a generic "config selector" kind of feature, and needs nothing else from grpc. This is in internal anyway, so it could be moved if desired. LMK if you disagree.

internal/resolver/safe_config_selector.go, line 66 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

I don't remember all the details now...
Should this be done in OnCommitted()?

No, that's not necessary. If we know resolver.ClientConn.UpdateState only returns after the old config selector is done being used, then ref counting of the clusters themselves can be done reliably via the resolver.

menghanl

Reviewed 4 of 5 files at r2.
Reviewable status: 7 of 9 files reviewed, 1 unresolved discussion (waiting on @dfawley and @menghanl)

internal/resolver/safe_config_selector.go, line 66 at r1 (raw file):

Previously, dfawley (Doug Fawley) wrote…

No, that's not necessary. If we know resolver.ClientConn.UpdateState only returns after the old config selector is done being used, then ref counting of the clusters themselves can be done reliably via the resolver.

What's the purpose of RPCConfig.OnComitted then? It's always nil now.

dfawley

Reviewable status: 7 of 9 files reviewed, 1 unresolved discussion (waiting on @menghanl)

internal/resolver/safe_config_selector.go, line 66 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

What's the purpose of RPCConfig.OnComitted then? It's always nil now.

It's only nil if the config selector doesn't set it. But we will use this for the resolver to ref count clusters.

menghanl

Reviewable status: 7 of 9 files reviewed, 1 unresolved discussion (waiting on @dfawley and @menghanl)

clientconn.go, line 647 at r2 (raw file):

		if sc, ok := s.ServiceConfig.Config.(*ServiceConfig); s.ServiceConfig.Err == nil && ok {
			cc.applyServiceConfigAndBalancer(sc, s.Addresses)
			if configSelector := iresolver.GetConfigSelector(s); configSelector != nil {

Handle the resolver setting configSelector to nil.

And should it skip the update if the new is same as the old (can be done inside SafeConfigSelector)

dfawley

Reviewable status: 7 of 9 files reviewed, 1 unresolved discussion (waiting on @menghanl)

clientconn.go, line 647 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Handle the resolver setting configSelector to nil.

And should it skip the update if the new is same as the old (can be done inside SafeConfigSelector)

Done for the first one.

It's unclear whether the second suggestion is worth doing. It requires an extra atomic read for every call, but saves a Wait() for the path where it doesn't change. We could benchmark this, but microbenchmarks may not be representative of real workloads, and the real-world savings is probably irrelevant, since resolver updates don't happen very frequently in practice. WDYT?

menghanl

Reviewable status: 6 of 9 files reviewed, 1 unresolved discussion (waiting on @dfawley and @menghanl)

clientconn.go, line 647 at r2 (raw file):

Previously, dfawley (Doug Fawley) wrote…

Done for the first one.

It's unclear whether the second suggestion is worth doing. It requires an extra atomic read for every call, but saves a Wait() for the path where it doesn't change. We could benchmark this, but microbenchmarks may not be representative of real workloads, and the real-world savings is probably irrelevant, since resolver updates don't happen very frequently in practice. WDYT?

I was thinking that when we do cluster ref-counting, deleting a cluster would result in a update with different balancer config, but the same config selector.
The clusters could be removed one by one when the qps is high, and this will cause multiple unnecessary ConfigSelector updates

dfawley · 2020-10-30T20:06:35Z

I was thinking that when we do cluster ref-counting, deleting a cluster would result in a update with different balancer config, but the same config selector.
The clusters could be removed one by one when the qps is high, and this will cause multiple unnecessary ConfigSelector updates

I'm not convinced. The number of updates here should be tiny and the impact on the system as a whole for each operation would also be quite small. Let's save this as a potential future optimization?

easwars · 2020-11-03T21:14:34Z

internal/resolver/config_selector.go

+
+// RPCInfo contains RPC information needed by a ConfigSelector.
+type RPCInfo struct {
+	Context context.Context // Contains headers, application timeout


Is it possible to move this context out of here and instead make it the first parameter to SelectConfig?

I really don't want to do that, as the config selector is not supposed to be blocking and having a Context parameter makes it look like it is fine to block. Context is only passed here for the data it contains (metadata & RPC deadline primarily). This is now awkward because gRPC-Go's API used context inappropriately, but it's better to pass it in a struct than as the first parameter to avoid assumptions about what it means IMO.

Agree with the blocking aspect. But my concern is the same as that in the other comment, that this context might become a dumping ground for stuff. And for someone reading this part of the code, it is not super clear what exact things are passed in this context. They would have to go look at the usages of this context field.

This is the user's context for the RPC. So it has whatever the user put in there, of which gRPC only cares about deadline and metadata (unless I'm forgetting something). I wouldn't advocate adding anything else there; we'd use CallOptions instead (what should have been used for metadata and deadline anyway so we wouldn't need to pass this). If we pull out the metadata and pass it instead, then we would be wasting resources (allocations & copies) for something that isn't used by the default config selector (service config).

Also, this is an internal API, so I think we get away with taking some liberties here.

Maybe add a comment to explain the rationale. Thanks.

internal/resolver/config_selector.go

menghanl

Reviewed 1 of 5 files at r2, 1 of 1 files at r3.
Reviewable status: 8 of 9 files reviewed, 3 unresolved discussions (waiting on @dfawley and @menghanl)

internal/resolver/safe_config_selector_test.go, line 64 at r3 (raw file):

		selectConfig: func(r RPCInfo) *RPCConfig {
			cs1Called <- struct{}{}
			if !reflect.DeepEqual(r, testRPCInfo) {

Nit: Use https://godoc.org/github.com/google/go-cmp/cmp#Diff for the diff

dfawley

Reviewable status: 7 of 9 files reviewed, 2 unresolved discussions (waiting on @dfawley, @easwars, and @menghanl)

internal/resolver/safe_config_selector_test.go, line 64 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Nit: Use https://godoc.org/github.com/google/go-cmp/cmp#Diff for the diff

Done

menghanl

LGTM

easwars · 2020-11-09T15:11:35Z

internal/resolver/safe_config_selector_test.go

+	return f.selectConfig(r)
+}
+
+func (s) TestSafeConfigSelector(t *testing.T) {


This test is not very easy to read. Lots of goroutines, lots of different time values which are used for waiting/sleeping. Could this be simplified?

I'm not sure..this test is intended to stress concurrency, since that's the whole point of the type. If you have suggestions for easier ways to cover the same things, then I can redo it.

Basically I want to cover:

cs1 used until a second UpdateConfigSelector call

cs2 used sometime after UpdateConfigSelector is called

UpdateConfigSelector blocks until no cs1 calls are in-flight

cs1 never called after first call to cs2

Return value of SelectConfig matches proper config selector's return value

I could make separate test cases for these, but my worry is that it would end up being 3+x longer because setting up (4) would require most of the same code as (1)-(3).

It does look like a lot to cover in a test. Maybe its just me who finds it hard to read this test :)

easwars · 2020-11-09T15:21:38Z

test/resolver_test.go

+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			select {
+			case sendConfigChan <- tc.config:


This seems to add an unnecessary coupling between test N and test N-1. Could this be avoided by making sure the expected state is reached at the end of test N-1?

We do test the expected state at the end of the previous test. Not the state of this channel, but it would be extremely surprising if the test case passes without consuming the config from the channel! This check is here mainly to avoid a hang in the event of some kind of weird bug that I can't actually even imagine. I could just delete the default case, I guess. But it's always good to fail quickly rather than hang.

Another option would be to recreate the client or update the config selector for every test case - then we wouldn't need a channel. WDYT?

Another option would be to recreate the client or update the config selector for every test case - then we wouldn't need a channel. WDYT?

Yes, I think that would be better.

Done. Getting rid of the channels is nicer.

easwars · 2020-11-09T15:24:01Z

test/resolver_test.go

+				t.Fatalf("no context received")
+			}
+
+			if want := "/grpc.testing.TestService/EmptyCall"; gotInfo.Method != want {


Could we move these two checks to inside of the select case in line 145? I feel grouping this with the code where we read gotInfo would make it easier to follow whats happening in the test.

Putting it inside the case increases the indent, which I don't like. Similar to above, the default in that select is only there for paranoia.

Actually, let me take advantage of our handy dandy channel wrappers here.

easwars · 2020-11-09T15:24:17Z

test/resolver_test.go

+				t.Errorf("gotInfo.Method = %q; want %q", gotInfo.Method, want)
+			}
+
+			if gotMD, _ := metadata.FromOutgoingContext(gotInfo.Context); !reflect.DeepEqual(tc.md, gotMD) {


Could this reflect.DeepEqual be avoided?

As in, you would prefer cmp.Diff? Done.

cmp.Equal would do.

The diffs help understanding the output I believe.

easwars · 2020-11-09T15:46:15Z

test/resolver_test.go

+			}
+
+			deadlineSent, _ := ctx.Deadline()
+			if tc.config != nil && tc.config.Context != nil {


There is a lot of special casing around config != nil and the corresponding context != nil. Could you split out the tests where these are expected to be nil and have helper functions for shared code between the tests?

Added wantMD and wantDeadline/wantTimeout instead. PTAL.

easwars · 2020-11-09T15:48:41Z

test/resolver_test.go

+				deadlineSent = time.Now().Add(*tc.config.MethodConfig.Timeout)
+			}
+			deadlineGot, _ := gotContext.Deadline()
+			if diff := deadlineGot.Sub(deadlineSent); diff > time.Second || diff < -time.Second {


Why do we check if the deadline is within this window?

Because the deadline is not precise when it is determined as a timeout (from "now", where "now" is not precisely controllable or knowable).

easwars · 2020-11-10T17:45:01Z

internal/resolver/safe_config_selector_test.go

+	cs1 := &fakeConfigSelector{
+		selectConfig: func(r RPCInfo) *RPCConfig {
+			cs1Called <- struct{}{}
+			if diff := cmp.Diff(r, testRPCInfo); diff != "" {


Might be better to use cmp.Equal instead of cmp.Diff here. I find the latter to be useful when comparing structured string output where a diff is very useful.

Here I think the structs probably benefit from the diff.

internal/resolver/safe_config_selector_appengine.go

easwars · 2020-11-10T18:26:56Z

internal/resolver/safe_config_selector_test.go

+	return f.selectConfig(r)
+}
+
+func (s) TestSafeConfigSelector(t *testing.T) {


It does look like a lot to cover in a test. Maybe its just me who finds it hard to read this test :)

easwars · 2020-11-10T18:27:54Z

test/resolver_test.go

+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			select {
+			case sendConfigChan <- tc.config:


Another option would be to recreate the client or update the config selector for every test case - then we wouldn't need a channel. WDYT?

Yes, I think that would be better.

easwars · 2020-11-10T18:28:37Z

test/resolver_test.go

+				t.Errorf("gotInfo.Method = %q; want %q", gotInfo.Method, want)
+			}
+
+			if gotMD, _ := metadata.FromOutgoingContext(gotInfo.Context); !reflect.DeepEqual(tc.md, gotMD) {


cmp.Equal would do.

easwars · 2020-11-10T18:34:33Z

test/resolver_test.go

+			}
+
+			onCommittedCalled = false
+			ctx = metadata.NewOutgoingContext(ctx, tc.md)


Is this not ctx := metadata.NewOutgoingContext(ctx, tc.md) by choice?

No, that's a mistake. Fixed.

test/resolver_test.go

internal/resolver/safe_config_selector_appengine.go

dfawley · 2020-11-11T21:47:13Z

PTAL at the recent changes, thanks.

easwars

LGTM.

clientconn.go

menghanl

Reviewed 1 of 2 files at r4.
Reviewable status: 5 of 10 files reviewed, 15 unresolved discussions (waiting on @dfawley, @easwars, and @menghanl)

clientconn.go, line 609 at r9 (raw file):

	}
	if cc.dopts.defaultServiceConfig != nil {
		cc.safeConfigSelector.UpdateConfigSelector(&defaultConfigSelector{cc})

Does the cc.sc != nil case also need a cc.safeConfigSelector.UpdateConfigSelector(&defaultConfigSelector{cc})?

We should probably change cc.applyServiceConfigAndBalancer to

func applyServiceConfigAndBalancerAndConfigSelector(sc, addrs, configSelector)

and call UpdateConfigSelector inside. So we don't have so many places to update safeConfigSelector.

Conceptually, config selector is a subset of method config, so it should be handled in the same place where cc.sc is updated.

And after this change, we will have:
cc.safeConfigSelector.UpdateConfigSelector() is called when

service config is updated
when resolver.Update contains an error
when recv from sc-channel (which nobody likes)

clientconn.go, line 645 at r9 (raw file):

	var ret error
	if cc.dopts.disableServiceConfig || s.ServiceConfig == nil {
		cc.maybeApplyDefaultServiceConfig(s.Addresses)

We are ignoring config selector from the resolver's Update when service config is disabled. Is that right?

dfawley

Reviewable status: 5 of 10 files reviewed, 15 unresolved discussions (waiting on @dfawley, @easwars, and @menghanl)

clientconn.go, line 647 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

I was thinking that when we do cluster ref-counting, deleting a cluster would result in a update with different balancer config, but the same config selector.
The clusters could be removed one by one when the qps is high, and this will cause multiple unnecessary ConfigSelector updates

(Addressed on github)

clientconn.go, line 609 at r9 (raw file):

Does the cc.sc != nil case also need a cc.safeConfigSelector.UpdateConfigSelector(&defaultConfigSelector{cc})?

No, because if we already have a service config, we also already have a config selector that matches it. SC and CS always belong as a pair.

applyServiceConfigAndBalancerAndConfigSelector

Hmm, this is a good idea, and I had that same though. The only problem with this is that when we call this with cc.sc as a parameter, we will also need to be able to indicate to it that the currently-active config selector should continue to be used. But that's hidden inside the SafeConfigSelector. We could pass nil to indicate this? PTAL.

clientconn.go, line 645 at r9 (raw file):

Previously, menghanl (Menghan Li) wrote…

We are ignoring config selector from the resolver's Update when service config is disabled. Is that right?

maybeApplyDefaultServiceConfig also applies the config selector. But yes, CS depends upon SC - conceptually, if there is no service config, there is no config to select.

…mance

menghanl

Reviewed 1 of 2 files at r4, 1 of 2 files at r5, 1 of 2 files at r7, 6 of 6 files at r10.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @dfawley, @easwars, and @menghanl)

clientconn.go, line 645 at r9 (raw file):

Previously, dfawley (Doug Fawley) wrote…

maybeApplyDefaultServiceConfig also applies the config selector. But yes, CS depends upon SC - conceptually, if there is no service config, there is no config to select.

So disabling service config will disable cluster picking for xDS?

clientconn.go, line 108 at r10 (raw file):

}

type defaultConfigSelector struct {

Nit: staticConfigSelector? constConfigSelector?

internal/resolver/config_selector.go, line 79 at r10 (raw file):

}

func (c *countingConfigSelector) Add() {

Why exported? Is there an interface for WaitGroup?

menghanl

Reviewed 5 of 6 files at r11.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @dfawley and @easwars)

dfawley

Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @dfawley, @easwars, and @menghanl)

clientconn.go, line 645 at r9 (raw file):

Previously, menghanl (Menghan Li) wrote…

So disabling service config will disable cluster picking for xDS?

Disabling service config will disable xDS (no LB policy!)

clientconn.go, line 108 at r10 (raw file):

Previously, menghanl (Menghan Li) wrote…

Nit: staticConfigSelector? constConfigSelector?

Or serviceConfig[Config]Selector? I think I prefer "default" of all these options, personally. Is there any specific reason(s) you don't like it?

internal/resolver/config_selector.go, line 79 at r10 (raw file):

Previously, menghanl (Menghan Li) wrote…

Why exported? Is there an interface for WaitGroup?

I assume this was on Add/Done/Wait before, not UpdateConfigSelector - no, that was just a bad habit. It's been removed.

menghanl

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @dfawley and @easwars)

clientconn.go, line 108 at r10 (raw file):

Previously, dfawley (Doug Fawley) wrote…

Or serviceConfig[Config]Selector? I think I prefer "default" of all these options, personally. Is there any specific reason(s) you don't like it?

I was thinking this struct is not "default". It's just a store for some config that doesn't change.
And now it's used for all configs (when config selector is not set), not just the default.

dfawley · 2020-11-17T20:37:21Z

I was thinking this struct is not "default". It's just a store for some config that doesn't change.
And now it's used for all configs (when config selector is not set), not just the default.

I think of this as the "default config selection logic". I.e. it pulls the method config from the service config based on the method name. "static" to me implies that it's some constant configuration (doesn't even vary based on method), which is not the case.

dfawley added Type: Feature New features or improvements in behavior no release notes labels Oct 26, 2020

dfawley requested a review from menghanl October 26, 2020 22:34

dfawley assigned menghanl Oct 26, 2020

menghanl suggested changes Oct 27, 2020

View reviewed changes

dfawley commented Oct 27, 2020

View reviewed changes

menghanl suggested changes Oct 30, 2020

View reviewed changes

dfawley commented Oct 30, 2020

View reviewed changes

menghanl suggested changes Oct 30, 2020

View reviewed changes

dfawley commented Oct 30, 2020

View reviewed changes

menghanl suggested changes Oct 30, 2020

View reviewed changes

easwars reviewed Nov 3, 2020

View reviewed changes

dfawley closed this Nov 4, 2020

dfawley reopened this Nov 4, 2020

menghanl approved these changes Nov 5, 2020

View reviewed changes

menghanl assigned easwars and unassigned menghanl Nov 5, 2020

dfawley commented Nov 6, 2020

View reviewed changes

menghanl approved these changes Nov 6, 2020

View reviewed changes

easwars reviewed Nov 9, 2020

View reviewed changes

easwars assigned dfawley and unassigned easwars Nov 9, 2020

easwars reviewed Nov 10, 2020

View reviewed changes

easwars approved these changes Nov 10, 2020

View reviewed changes

internal/resolver/safe_config_selector_appengine.go Outdated Show resolved Hide resolved

dfawley assigned menghanl and easwars and unassigned dfawley Nov 11, 2020

easwars reviewed Nov 11, 2020

View reviewed changes

clientconn.go Outdated Show resolved Hide resolved

menghanl suggested changes Nov 12, 2020

View reviewed changes

dfawley commented Nov 12, 2020

View reviewed changes

dfawley added 15 commits November 12, 2020 15:32

xds: add ConfigSelector to support RouteAction timeouts

68f49fc

add forgotten t.Fatalf

56bc6f4

review comments; wire up OnCommitted

27fb911

allow config selector to go back to default

5091ef8

add legal disclaimer to RPCInfo.Context

58f21c4

use cmp.Diff

c8a5d31

test tweaks

bbaf52e

use tc.wantMD for real

e8b7235

address review comments

6eadc99

simplify map loop & make vet happy

5b51a85

remove nil check on counting config selector pointer

138cbc0

ensure we always set a config selector

0824fdc

one more error case where cs wasn't set

701201d

add config selector to applyServiceConfigAndBalancer

ab04a87

fix deadlock; fix artificial waitgroup 'race' and also improve perfor…

9bac1a6

…mance

dfawley force-pushed the config_selector branch from f58f4be to 9bac1a6 Compare November 12, 2020 23:33

dfawley added 2 commits November 12, 2020 16:24

make staticcheck happy

f6c6820

fix typo

4bf0ed5

menghanl suggested changes Nov 16, 2020

View reviewed changes

use rwmutex as SafeConfigSelector; add relevant benchmarks

61a4ae0

menghanl approved these changes Nov 17, 2020

View reviewed changes

dfawley commented Nov 17, 2020

View reviewed changes

menghanl approved these changes Nov 17, 2020

View reviewed changes

dfawley merged commit b88744b into grpc:master Nov 17, 2020

dfawley deleted the config_selector branch November 17, 2020 21:22

davidkhala pushed a commit to Hyperledger-TWGC/grpc that referenced this pull request Dec 7, 2020

xds: add ConfigSelector to support RouteAction timeouts (grpc#3991)

f8318df

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021

xds: add ConfigSelector to support RouteAction timeouts #3991

xds: add ConfigSelector to support RouteAction timeouts #3991

Conversation

dfawley commented Oct 26, 2020 • edited

menghanl left a comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

dfawley commented Oct 30, 2020

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

menghanl left a comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

menghanl 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

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

dfawley commented Nov 11, 2020

easwars left a comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

dfawley commented Nov 17, 2020

dfawley commented Oct 26, 2020 •

edited