priority: the implementation #4070

menghanl · 2020-12-01T21:58:42Z

Functionality is similar to the priority inside EDS.

easwars

Is it possible to split this into smaller pieces. I understand it might be difficult, but this PR is super huge.

xds/internal/balancer/priority/balancer.go

menghanl

I didn't find an easy way to split this... I can add the priority handling methods first, but that will be a PR with no running code, or meaningful tests. And that PR would probably be harder to understand.

xds/internal/balancer/priority/balancer.go

easwars

I have looked at very little so far, but I thought it might be better to send my comments as and when I have them instead of accumulating them since it might take a while before I get done with the whole thing.

And apologize for the delay so far.

easwars · 2020-12-02T22:30:12Z

xds/internal/balancer/priority/balancer.go

+	logger           *grpclog.PrefixLogger
+	cc               balancer.ClientConn
+	bg               *balancergroup.BalancerGroup
+	ctx              context.Context


How about a grpcsync.Event instead of a context?

easwars · 2020-12-02T22:33:23Z

xds/internal/balancer/priority/balancer_priority.go

+
+	priority, ok := pb.childToPriority[childName]
+	if !ok {
+		pb.logger.Infof("priority: received picker update with unknown child")


Include childName in the log message.

Also, this log message should be error/warning.

easwars · 2020-12-02T22:35:21Z

xds/internal/balancer/priority/balancer_priority.go

+	}
+
+	if pb.childInUse == "" {
+		pb.logger.Infof("priority: no childInUse when picker update is received", pb.childInUse)


Prefer avoiding the use of variable names in log messages.

Also childInUse is known to be empty here. So, why print it? And there is no format string for it anyways. Should vet be catching these?

Again, this should be error/warning.

easwars · 2020-12-02T22:37:38Z

xds/internal/balancer/priority/balancer_priority.go

+	}
+
+	// priorityInUse is higher than this priority.
+	if pb.priorityInUse < priority {


Didn't we have a type for priority which made comparisons more readable? Should we be using that?

Same here for this log message and every other log message:

if it is an error condition, please use error/warning

capture as much of the state in the message. (the received priority is not printed here)

do not use variable names in the log message.

I found using that type doesn't gain us much, but is very cumbersome.

Log messages updated.

easwars · 2020-12-02T22:39:15Z

xds/internal/balancer/priority/balancer_priority.go

+	case connectivity.Connecting:
+		pb.handlePriorityWithNewStateConnecting(child, priority, oldState)
+	default:
+		// New state is Idle, should never happen. Don't forward.


Maybe an error/warning log here?

There should be connectivity state change in child balancers. I think we can safely ignore this here.

xds/internal/balancer/priority/balancer.go

easwars · 2020-12-10T20:09:40Z

xds/internal/balancer/priority/balancer.go

+	cancel           context.CancelFunc
+	childStateUpdate *buffer.Unbounded
+
+	config *lbConfig


The only use of this field seems to be at the end of UpdateClientConnState where we write to it. Do we need this? Is there a plan to use it in the future? Or am I missing something?

easwars · 2020-12-10T20:13:12Z

xds/internal/balancer/priority/balancer.go

+	pb.priorities = make([]string, 0, len(newConfig.Priorities))
+	pb.childToPriority = make(map[string]int)
+	for pi, pName := range newConfig.Priorities {
+		pb.priorities = append(pb.priorities, pName)


Can't we directly assign pb.priorities = newConfig.Priorities? If not, can we say pb.priorities[pi] = pName since you have already made the slice as big as required.

Hmm, yeah, directly assigning works! Done

easwars · 2020-12-10T20:15:59Z

xds/internal/balancer/priority/balancer.go

+func newPriorityBalancer(cc balancer.ClientConn) *priorityBalancer {
+	b := &priorityBalancer{
+		cc:               cc,
+		childToPriority:  make(map[string]int),


I'm guessing we don't need to make the childToPriority map here since it is always being made again in UpdateClientConnState.

But if the read in handleChildStateUpdate() happens before the write (in an error case), it may panic due to nil.

easwars · 2020-12-10T20:36:44Z

xds/internal/balancer/priority/balancer_child.go

+
+type childBalancer struct {
+	name   string
+	parent *priorityBalancer


The only reason parent seems to be used here is to access the balancerGroup. Could we instead have the operations on the balancerGroup handled directly by the priorityBalancer. If that would make things complex, at least we can directly accept the balancerGroup here instead of the parent. What do you think?

I thought the children also need the logger from parent. But it doesn't seem to call it.
I'm keeping it in case we need info logs later

easwars · 2020-12-14T17:55:56Z

xds/internal/balancer/priority/balancer.go

+	for name, newSubConfig := range newConfig.Children {
+		bb := balancer.Get(newSubConfig.Config.Name)
+		if bb == nil {
+			pb.logger.Warningf("balancer name %v from config is not registered", newSubConfig.Config.Name)


Why is this only a warning and not an error? Would it be better to check this in ParseConfig and if we find an unregistered child policy, should we fail there?

Service config parsing should already checked this, and would not accept unregistered balancers. So this should never happen.

In that case, I'm in favor of Error. This will make tests fail if it happens, and since it "can't happen", it doesn't matter either way for production.

Service config parsing should already checked this, and would not accept unregistered balancers. So this should never happen.

Sorry for coming back to this after a long break.
Where in service config parsing do we check if the balancer name is registered? I don't see such a check in the parseConfig() method defined in config.go.

This type BalancerConfig is a specially defined type

grpc-go/xds/internal/balancer/priority/config.go

Lines 29 to 31 in f005af0

type child struct {

Config *internalserviceconfig.BalancerConfig

}

Its UnmarshalJSON is where the magic happens:

grpc-go/internal/serviceconfig/serviceconfig.go

Lines 49 to 57 in f005af0

// UnmarshalJSON implements the json.Unmarshaler interface.

//

// ServiceConfig contains a list of loadBalancingConfigs, each with a name and

// config. This method iterates through that list in order, and stops at the

// first policy that is supported.

// - If the config for the first supported policy is invalid, the whole service

// config is invalid.

// - If the list doesn't contain any supported policy, the whole service config

// is invalid.

easwars · 2020-12-14T20:16:38Z

xds/internal/balancer/priority/balancer_priority.go

+// balancer state (started or not) is in sync with the priorities (even in
+// tricky cases where a child is moved from a priority to another).
+//
+// It's guaranteed that after this function returns:


Very detailed function level comments become hard to keep in sync with the code as the code evolves. Do you think it makes more sense to move them closer to the actual code?

Here and in some of the other functions.

Listing the pre/post-conditions is always fine, and a good idea.

"Steps", if they are essentially documenting "how does this function work", should be in-line or at the top of the function (inside the function decl; though this doesn't matter much for unexported functions).

easwars · 2020-12-14T20:18:28Z

xds/internal/balancer/priority/balancer_priority.go

+	for p, name := range pb.priorities {
+		child, ok := pb.children[name]
+		if !ok {
+			pb.logger.Warningf("child with name %q is not found in children", name)


This should ideally not be possible. We should check it in ParseConfig and UpdateClientConnState.

This is check in config parsing. This log is just in case.

If we think it should be impossible, this should be Errorf; if it happens in tests, it will fail.

easwars · 2020-12-14T20:22:53Z

xds/internal/balancer/priority/balancer_priority.go

+			}
+		})
+	}
+


Nix newline.

easwars · 2020-12-14T20:28:18Z

xds/internal/balancer/priority/balancer_child.go

+// sendUpdate sends the addresses and config to the child balancer.
+func (cb *childBalancer) sendUpdate() {
+	// TODO: handle aggregate returned error.
+	cb.parent.bg.UpdateClientConnState(cb.name, balancer.ClientConnState{


Why do we ignore this error?

Added logging. We don't have a good way to aggregate the error now.

easwars · 2020-12-14T21:16:45Z

xds/internal/balancer/priority/balancer_priority.go

+
+	// Update state in child. The updated picker will be sent to parent later if
+	// necessary.
+	child, ok := pb.children[childName]


Is this condition possible at all?

Not in a normal case I think.

easwars · 2020-12-14T22:43:33Z

xds/internal/balancer/priority/balancer_test.go

+
+func subConnFromPicker(p balancer.Picker) func() balancer.SubConn {
+	return func() balancer.SubConn {
+		scst, _ := p.Pick(balancer.PickInfo{})


Why is the error from Pick ignored? If required, testutils.IsRoundRobin should be fixed to accept a function f func() (balancer.SubConn, error).

The error should always be nil here. Otherwise the test will fail because the returned subconns don't match.

I changed this function to take a testing.T, and fatal if err is not nil.

easwars · 2020-12-14T22:50:46Z

xds/internal/balancer/priority/balancer_test.go

+	testRRBalancerName    = "another-round-robin"
+)
+
+type anotherRR struct {


Why do we need this balancer? Why can't we use pick-first or passthrough when testing changing of child policies?

Using RR is simpler to test the child policy's behavior. It makes one SubConn per address, but passthrough makes one SubConn for all the addresses.

easwars · 2020-12-14T22:51:11Z

xds/internal/balancer/priority/balancer_test.go

+	defer pb.Close()
+
+	// Two children, with priorities [0, 1], each with one backend.
+	pb.UpdateClientConnState(balancer.ClientConnState{


Please do not ignore errors. We should catch them and call Fatal.
This comment applies to all calls to UpdateClientConnState

easwars · 2020-12-14T22:56:44Z

xds/internal/balancer/priority/balancer_test.go

+	}
+
+	// Remove p2, no updates.
+	pb.UpdateClientConnState(balancer.ClientConnState{


If some of these client conn updates are the same across tests, maybe they can be declared as vars, and shared between tests.

The values are the same, but they are not "consts" (unlike the test addresses or test proto messages). I think keeping these in the tests makes the test steps clear

xds/internal/balancer/priority/balancer.go

dfawley · 2021-01-08T18:26:31Z

xds/internal/balancer/priority/balancer.go

+	for name, newSubConfig := range newConfig.Children {
+		bb := balancer.Get(newSubConfig.Config.Name)
+		if bb == nil {
+			pb.logger.Warningf("balancer name %v from config is not registered", newSubConfig.Config.Name)


In that case, I'm in favor of Error. This will make tests fail if it happens, and since it "can't happen", it doesn't matter either way for production.

dfawley · 2021-01-08T18:33:33Z

xds/internal/balancer/priority/balancer.go

+	priorityInitTimer *time.Timer
+}
+
+func (pb *priorityBalancer) UpdateClientConnState(s balancer.ClientConnState) error {


Nit: p or b? pb makes me think of the common protobuf abbreviation every time I see it.

dfawley · 2021-01-08T18:35:08Z

xds/internal/balancer/priority/balancer.go

+
+type priorityBB struct{}
+
+func (pbb *priorityBB) Build(cc balancer.ClientConn, _ balancer.BuildOptions) balancer.Balancer {


FWIW since there are no fields in the struct, I believe removing all the pointers around priorityBB is more efficient and more "Go-standard".

Removed * and the receiver name.

dfawley · 2021-01-14T23:21:40Z

xds/internal/balancer/priority/balancer_priority.go

+	pb.mu.Lock()
+	defer pb.mu.Unlock()


Might it be important to acquire the lock before checking done?

Right. Fixed.

dfawley · 2021-01-14T23:22:51Z

xds/internal/balancer/priority/balancer_priority.go

+
+	// priorityInUse is higher than this priority.
+	if pb.priorityInUse < priority {
+		// Lower priorities should all be closed, this is an unexpected update.


Unexpected how? If we think it's impossible, Error log instead.

This can happen if the child policy somehow sends an update after we tell it to stop. Not necessary an error though.

dfawley · 2021-01-14T23:23:04Z

xds/internal/balancer/priority/balancer_priority.go

+	// priorityInUse is higher than this priority.
+	if pb.priorityInUse < priority {
+		// Lower priorities should all be closed, this is an unexpected update.
+		pb.logger.Warningf("priority: received picker update from priority %v,  lower then priority in use %v", priority, pb.priorityInUse)


"lower thAn"

dfawley · 2021-01-14T23:26:36Z

xds/internal/balancer/priority/utils.go

+	for i, v := range a {
+		if v != b[i] {
+			return false
+		}
+	}


Remove v and save some small amount of copying?

for i := range a { if a[i] != b[i] { return false } }

dfawley · 2021-01-14T23:28:31Z

xds/internal/balancer/priority/utils_test.go

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := equalStringSlice(tt.a, tt.b); got != tt.want {
+				t.Errorf("equalStringSlice() = %v, want %v", got, tt.want)


Print the inputs for easier debugging:

t.Errorf("equalStringSlice(%v, %v) = %v, want %v", tt.a, tt.b, got, tt.want)

menghanl

Thanks for the review. PTAL.

menghanl · 2021-01-21T23:14:56Z

xds/internal/balancer/priority/balancer.go

@@ -0,0 +1,214 @@
+/*
+ *
+ * Copyright 2020 gRPC authors.


xds/internal/balancer/priority/balancer.go

menghanl · 2021-01-21T23:17:13Z

xds/internal/balancer/priority/balancer.go

+
+type priorityBB struct{}
+
+func (pbb *priorityBB) Build(cc balancer.ClientConn, _ balancer.BuildOptions) balancer.Balancer {


Removed * and the receiver name.

xds/internal/balancer/priority/balancer.go

menghanl · 2021-01-21T23:20:07Z

xds/internal/balancer/priority/balancer.go

+	priorityInitTimer *time.Timer
+}
+
+func (pb *priorityBalancer) UpdateClientConnState(s balancer.ClientConnState) error {


menghanl · 2021-01-22T00:10:41Z

xds/internal/balancer/priority/utils_test.go

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := equalStringSlice(tt.a, tt.b); got != tt.want {
+				t.Errorf("equalStringSlice() = %v, want %v", got, tt.want)


menghanl · 2021-01-22T05:53:59Z

xds/internal/balancer/priority/balancer_test.go

+	testRRBalancerName    = "another-round-robin"
+)
+
+type anotherRR struct {


Using RR is simpler to test the child policy's behavior. It makes one SubConn per address, but passthrough makes one SubConn for all the addresses.

menghanl · 2021-01-22T05:56:49Z

xds/internal/balancer/priority/balancer_test.go

+	}
+
+	// Remove p2, no updates.
+	pb.UpdateClientConnState(balancer.ClientConnState{


The values are the same, but they are not "consts" (unlike the test addresses or test proto messages). I think keeping these in the tests makes the test steps clear

menghanl · 2021-01-22T06:02:36Z

xds/internal/balancer/priority/balancer_test.go

+
+func subConnFromPicker(p balancer.Picker) func() balancer.SubConn {
+	return func() balancer.SubConn {
+		scst, _ := p.Pick(balancer.PickInfo{})


The error should always be nil here. Otherwise the test will fail because the returned subconns don't match.

I changed this function to take a testing.T, and fatal if err is not nil.

menghanl · 2021-01-22T06:06:36Z

xds/internal/balancer/priority/balancer_test.go

+	defer pb.Close()
+
+	// Two children, with priorities [0, 1], each with one backend.
+	pb.UpdateClientConnState(balancer.ClientConnState{


menghanl

Thanks for the review. PTAL.

menghanl

Thanks for the review. All done. PTAL.

xds/internal/balancer/priority/balancer.go

menghanl · 2021-02-03T19:32:08Z

xds/internal/balancer/priority/balancer.go

-func (pb *priorityBalancer) UpdateState(childName string, state balancer.State) {
-	pb.childStateUpdate.Put(&balancerStateWithPriority{
+func (b *priorityBalancer) UpdateState(childName string, state balancer.State) {
+	b.childStateUpdate.Put(&balancerStateWithChildName{


I assume you meant the struct name, but not sure.
So I renamed both the channel and the struct. :)

xds/internal/balancer/priority/balancer_priority.go

easwars

Mostly minor nits.

easwars · 2021-02-04T08:53:44Z

xds/internal/balancer/priority/balancer_priority.go

+func (b *priorityBalancer) handleChildStateUpdate(childName string, s balancer.State) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	if b.done.HasFired() {


Nit: Should we check done before grabbing the lock?

It needs to be inside mutex, otherwise it may race with close().

easwars · 2021-02-04T08:56:22Z

xds/internal/balancer/priority/balancer_priority.go

+
+	priority, ok := b.childToPriority[childName]
+	if !ok {
+		b.logger.Warningf("priority: received picker update with unknown child %v", childName)


This log and the next one should be Error I guess.

easwars · 2021-02-04T09:04:20Z

xds/internal/balancer/priority/balancer_priority.go

+	b.stopPriorityInitTimer()
+
+	// priorityInUse is lower than this priority, switch to this.
+	if b.priorityInUse > priority {


Do we really need this check here? switchToChild does handle the case where priorityInUse is the same as priority, by doing nothing.

That's right. But keep it just so that the next Infof is accurate?

easwars · 2021-02-04T09:15:13Z

xds/internal/balancer/priority/balancer_test.go

+	return func() balancer.SubConn {
+		scst, err := p.Pick(balancer.PickInfo{})
+		if err != nil {
+			t.Fatalf("unexpected error from picker.Pick: %v", err)


Why do we need to pass testing.T to this function? This function always seems to be called as the second argument to testutils.IsRoundRobin(), and if the subConns don't match, the test body goes ahead and calls t.Error/Fatal.

If Pick() returns an error, testutils.IsRoundRobin() will fail because the returned sc is nil, but we don't know why Pick() error'ed.

This t.Fatalf prints the error.

easwars · 2021-02-04T09:28:17Z

xds/internal/balancer/priority/balancer_test.go

+
+// A test balancer that updates balancer.State inline when handling ClientConn
+// state.
+type testInlineUpdateBalancerBuilder struct{}


Can we use the stub balancer here? We should really be trying to use the stub balancer wherever possible instead of sprinkling our codebase with test balancer implementations.

Good call. Done.

easwars · 2021-02-04T09:28:43Z

xds/internal/balancer/priority/balancer_test.go

+	grpctest.RunSubTests(t, s{})
+}
+
+var (


Nit: remove the paranthesis

menghanl

Thanks for the review. All done. PTAL.

menghanl · 2021-02-04T18:47:56Z

xds/internal/balancer/priority/balancer_priority.go

+func (b *priorityBalancer) handleChildStateUpdate(childName string, s balancer.State) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	if b.done.HasFired() {


It needs to be inside mutex, otherwise it may race with close().

menghanl · 2021-02-04T18:48:57Z

xds/internal/balancer/priority/balancer_priority.go

+
+	priority, ok := b.childToPriority[childName]
+	if !ok {
+		b.logger.Warningf("priority: received picker update with unknown child %v", childName)


menghanl · 2021-02-04T18:51:41Z

xds/internal/balancer/priority/balancer_priority.go

+	b.stopPriorityInitTimer()
+
+	// priorityInUse is lower than this priority, switch to this.
+	if b.priorityInUse > priority {


That's right. But keep it just so that the next Infof is accurate?

menghanl · 2021-02-04T18:53:00Z

xds/internal/balancer/priority/balancer_test.go

+	grpctest.RunSubTests(t, s{})
+}
+
+var (


menghanl · 2021-02-04T18:55:14Z

xds/internal/balancer/priority/balancer_test.go

+	return func() balancer.SubConn {
+		scst, err := p.Pick(balancer.PickInfo{})
+		if err != nil {
+			t.Fatalf("unexpected error from picker.Pick: %v", err)


If Pick() returns an error, testutils.IsRoundRobin() will fail because the returned sc is nil, but we don't know why Pick() error'ed.

This t.Fatalf prints the error.

menghanl · 2021-02-04T19:02:32Z

xds/internal/balancer/priority/balancer_test.go

+
+// A test balancer that updates balancer.State inline when handling ClientConn
+// state.
+type testInlineUpdateBalancerBuilder struct{}


Good call. Done.

menghanl requested a review from easwars December 1, 2020 21:58

menghanl assigned easwars Dec 1, 2020

menghanl added the Type: Feature New features or improvements in behavior label Dec 1, 2020

easwars added this to the 1.35 Release milestone Dec 2, 2020

easwars reviewed Dec 2, 2020

View reviewed changes

xds/internal/balancer/priority/balancer.go Outdated Show resolved Hide resolved

menghanl commented Dec 2, 2020

View reviewed changes

xds/internal/balancer/priority/balancer.go Outdated Show resolved Hide resolved

easwars requested changes Dec 10, 2020

View reviewed changes

easwars reviewed Dec 10, 2020

View reviewed changes

easwars reviewed Dec 14, 2020

View reviewed changes

easwars assigned menghanl and unassigned easwars Dec 14, 2020

easwars reviewed Dec 14, 2020

View reviewed changes

menghanl force-pushed the priority_policy_balancer branch from 56941a4 to 5da3571 Compare January 6, 2021 22:55

menghanl assigned dfawley and unassigned menghanl Jan 6, 2021

menghanl requested a review from dfawley January 6, 2021 22:56

menghanl force-pushed the priority_policy_balancer branch from 5da3571 to 0264721 Compare January 7, 2021 22:24

menghanl modified the milestones: 1.35 Release, 1.36 Release Jan 8, 2021

dfawley reviewed Jan 14, 2021

View reviewed changes

dfawley assigned menghanl and unassigned dfawley Jan 15, 2021

menghanl commented Jan 22, 2021

View reviewed changes

menghanl force-pushed the priority_policy_balancer branch from 80c2423 to efd0418 Compare January 22, 2021 06:10

dfawley assigned easwars and dfawley and unassigned menghanl Jan 29, 2021

menghanl unassigned easwars Jan 29, 2021

dfawley assigned menghanl Feb 2, 2021

menghanl added 10 commits February 3, 2021 11:27

[priority_policy_balancer] the balancer

ee5910e

[priority_policy_balancer] the tests

98339e6

[priority_policy_balancer] clean

1395159

[priority_policy_balancer] clean

bddc3e0

[priority_policy_balancer] Test ready to connecting

3e08067

[priority_policy_balancer] c1.1

0d5f80e

[priority_policy_balancer] copyright

c03a3ff

[priority_policy_balancer] cc1

b5b1283

[priority_policy_balancer] fix timer race

d4684db

[priority_policy_balancer] fix test comments

3ec2e11

menghanl commented Feb 3, 2021

View reviewed changes

[priority_policy_balancer] fix review

44e47b5

menghanl force-pushed the priority_policy_balancer branch from efd0418 to 44e47b5 Compare February 3, 2021 19:39

menghanl assigned dfawley and unassigned menghanl Feb 3, 2021

dfawley approved these changes Feb 3, 2021

View reviewed changes

easwars approved these changes Feb 4, 2021

View reviewed changes

easwars assigned menghanl and unassigned dfawley Feb 4, 2021

menghanl added 2 commits February 4, 2021 10:56

[priority_policy_balancer] review 1

80e6d1b

[priority_policy_balancer] stub balancer

c243a92

menghanl commented Feb 4, 2021

View reviewed changes

menghanl assigned easwars and unassigned menghanl Feb 4, 2021

easwars modified the milestones: 1.36 Release, 1.37 Release Feb 10, 2021

menghanl merged commit ad24ab5 into grpc:master Feb 10, 2021

menghanl deleted the priority_policy_balancer branch February 10, 2021 18:38

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021

	type child struct {
	Config *internalserviceconfig.BalancerConfig
	}

	// UnmarshalJSON implements the json.Unmarshaler interface.
	//
	// ServiceConfig contains a list of loadBalancingConfigs, each with a name and
	// config. This method iterates through that list in order, and stops at the
	// first policy that is supported.
	// - If the config for the first supported policy is invalid, the whole service
	// config is invalid.
	// - If the list doesn't contain any supported policy, the whole service config
	// is invalid.


		type priorityBB struct{}

		func (pbb *priorityBB) Build(cc balancer.ClientConn, _ balancer.BuildOptions) balancer.Balancer {

priority: the implementation #4070

priority: the implementation #4070

Conversation

menghanl commented Dec 1, 2020

easwars left a comment

Choose a reason for hiding this comment

menghanl left a comment • edited

Choose a reason for hiding this comment

easwars 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

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

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

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

menghanl left a comment

menghanl left a comment •

edited