workflow: Modify the unit test for resharding workflow. Control the workflow through manager.

[wangyipei01]

This change is 

[mberlin-bot]

Change looks very good! :) I've left a few comments regarding style issues and how to simplify the topo data creation.

---

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

go/vt/vtctld/workflow.go, line 44 at r1 (raw file):

		// Register the Horizontal Resharding workflow.
		resharding.Register()

Please don't remove this.

With this change your workflow is no longer registered by default because your package isn't imported (transitively) by the "vtctld" package. Because of that your e2e test should fail now.

The other reason is that we want to keep things consistent. Either we register all workflows here or now. Let's register all here.

I suggest you keep this and instead do the register in the init() function as well like you did in test_workflow.go.

---

go/vt/workflow/manager.go, line 462 at r1 (raw file):

runningworkflow

runningWorkflow

I would not mention "runningWorkflow" because it's an implementation detail. The user cares only about the "Workflow" object. Therefore, I propose:

GetWorkflowForTesting returns the "Workflow" object for the running workflow identified by "uuid".

---

go/vt/workflow/manager.go, line 463 at r1 (raw file):

The method is used when injecting a mock interface into a manager
// created workflow in unit test.

simpler:

The method is used in unit tests to inject mocks.

---

go/vt/workflow/resharding/horizontal_resharding_workflow.go, line 318 at r1 (raw file):

	hw.topoServer = manager.TopoServer()
	hw.manager = manager
	if hw.wr == nil {

This is not obvious and would require a comment.

Instead of initializing the fields here, please do this in Instantiate() instead. That's the right place to do it.

For that, you'll have to add Manager to Instantiate() as well. Alain said yesterday that we should do that anyway, right?

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 44 at r1 (raw file):

	m := workflow.NewManager(ts)
	// Run the manager in the background.
	wg, cancel, _ := startManager(t, m)

Some comments on startManager() which is in parallel_runner_test.go:

• please remove t as parameter because it's not used
• please call wg.Done() via defer because that's the usual pattern. Additionally, m.Run() could panic and Done would not be called.
• please swap the return values ctx and cancel because that's the same order which WithCancel returns

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 53 at r1 (raw file):

t.Errorf

t.Fatalf

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 65 at r1 (raw file):

// TODO: implemented in parallel_runner_test.

TODO notes please always on a separate line and in the format TODO(ldap) or TODO(b/...).

But I think you can actually remove this comment?

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 65 at r1 (raw file):

verifyWorkflowSuccess

Please rename this to "verifyAllTasksDone" because that better describes what the function does.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 65 at r1 (raw file):

t

Please don't pass in the testing.T object. This way you cannot trace back which test method called this function.

Instead, return an error:

if err := verifyAllTasksDone(context.Background(), ts, uuid); err != nil {
  t.Fatal(err)
}

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 87 at r1 (raw file):

	// It is hard to set the expect call using the workflow's context since
	// it is only assigned when calling workflow.Run. Therefore, we only check
	// the type of the context argument in expect calls.

This is not necessary. It's unlikely that somebody is passing not the ctx and nil instead.

To keep things simple, please use the Any matcher further down instead please: https://godoc.org/github.com/golang/mock/gomock#Any

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 93 at r1 (raw file):

	defer mCancel()

	ctxMatcher := typeMatcher{workflowCtx}

FYI: You could have passed in context.Background() as well here because it's the same type?

But as I said above, just don't check it.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 115 at r1 (raw file):

t.Errorf

t.Fatalf

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 117 at r1 (raw file):

		t.Errorf("CreateKeyspace: %v", err)
	}
	createShard(ctx, t, ts.Impl, keyspace, "0", true)

You can simplify this.

Call ts.CreateShard() (i.e. do not use the Impl) for each shard which sets the serving type.

The advantage of this approach is that you're not duplicating code from the topology package e.g. for setting the keyrange.

It will also automatically not set the serving types for the two later shards because it detects that shard "0" is already serving that key range.

---

Comments from Reviewable

[wangyipei01-bot]

Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

go/vt/vtctld/workflow.go, line 44 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Please don't remove this.

With this change your workflow is no longer registered by default because your package isn't imported (transitively) by the "vtctld" package. Because of that your e2e test should fail now.

The other reason is that we want to keep things consistent. Either we register all workflows here or now. Let's register all here.

I suggest you keep this and instead do the register in the init() function as well like you did in test_workflow.go.

Done.

---

go/vt/workflow/manager.go, line 462 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

runningworkflow

runningWorkflow

I would not mention "runningWorkflow" because it's an implementation detail. The user cares only about the "Workflow" object. Therefore, I propose:

GetWorkflowForTesting returns the "Workflow" object for the running workflow identified by "uuid".

Done.

---

go/vt/workflow/manager.go, line 463 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

The method is used when injecting a mock interface into a manager
// created workflow in unit test.

simpler:

The method is used in unit tests to inject mocks.

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow.go, line 318 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

This is not obvious and would require a comment.

Instead of initializing the fields here, please do this in Instantiate() instead. That's the right place to do it.

For that, you'll have to add Manager to Instantiate() as well. Alain said yesterday that we should do that anyway, right?

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 44 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Some comments on startManager() which is in parallel_runner_test.go:

• please remove t as parameter because it's not used
• please call wg.Done() via defer because that's the usual pattern. Additionally, m.Run() could panic and Done would not be called.
• please swap the return values ctx and cancel because that's the same order which WithCancel returns

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 53 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

t.Errorf

t.Fatalf

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 65 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

// TODO: implemented in parallel_runner_test.

TODO notes please always on a separate line and in the format TODO(ldap) or TODO(b/...).

But I think you can actually remove this comment?

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 65 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

verifyWorkflowSuccess

Please rename this to "verifyAllTasksDone" because that better describes what the function does.

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 65 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

t

Please don't pass in the testing.T object. This way you cannot trace back which test method called this function.

Instead, return an error:

if err := verifyAllTasksDone(context.Background(), ts, uuid); err != nil {
  t.Fatal(err)
}

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 87 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

This is not necessary. It's unlikely that somebody is passing not the ctx and nil instead.

To keep things simple, please use the Any matcher further down instead please: https://godoc.org/github.com/golang/mock/gomock#Any

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 93 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

FYI: You could have passed in context.Background() as well here because it's the same type?

But as I said above, just don't check it.

Solved in above comment's reply.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 115 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

t.Errorf

t.Fatalf

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 117 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

You can simplify this.

Call ts.CreateShard() (i.e. do not use the Impl) for each shard which sets the serving type.

The advantage of this approach is that you're not duplicating code from the topology package e.g. for setting the keyrange.

It will also automatically not set the serving types for the two later shards because it detects that shard "0" is already serving that key range.

Done.

---

Comments from Reviewable

[mberlin-bot]

Some minor comments. Almost there :)

---

Reviewed 9 of 9 files at r2.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

go/vt/workflow/manager.go, line 463 at r2 (raw file):

The method is used in unit test to inject mocks

nits:

• "unit tests" (plural)
• missing period :)

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 17 at r2 (raw file):

	"github.com/youtube/vitess/go/vt/wrangler"

	_ "github.com/youtube/vitess/go/vt/tabletmanager/grpctmclient"

I think in Go, it's a convention that imports with a blank identifier need a comment.

Even if not, a comment is useful here to educate the reader why we're doing this.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 52 at r2 (raw file):

mctx

Please use ctx here. The manager Context (mctx) is used internally by the Manager and controls its lifetime. If the context is done, the manager should shut down and stop the workflow.

On the other hand, Create() and Start() below use the passed in context only for operations on the topology i.e. it's a different usage. Therefore, let's use ctx. If mctx should be used, there shouldn't be a parameter for a Context in the first place.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 64 at r2 (raw file):

	hw.wr = mockWranglerInterface

	fmt.Println("workflow created")

Please remove debug statements.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 66 at r2 (raw file):

mctx

ctx

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 67 at r2 (raw file):

testworkflow

resharding workflow

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 71 at r2 (raw file):

mctx

ctx

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 72 at r2 (raw file):

	// Wait for the workflow to end.
	m.Wait(mctx, uuid)
	verifyAllTasksDone(ctx, ts, uuid)

Please check the return value and fail the test if != nil.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 91 at r2 (raw file):

ctx context.Context,

unused, please remove

---

go/vt/workflow/resharding/parallel_runner_test.go, line 24 at r2 (raw file):

ctx

Same comment as in the other file. Please don't use the Manager context for the calls below and instead use context.Background().

---

go/vt/workflow/resharding/parallel_runner_test.go, line 41 at r2 (raw file):

t.Fatalf("verify all tasks done failed: %v", err)

FYI:

t.Fatal(err) would be good enough here. The test failure gives you the exact code location and should already have enough error details from the return value.

Can you please change this throughout the file?

---

Comments from Reviewable

[wangyipei01-bot]

Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

go/vt/workflow/manager.go, line 463 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

The method is used in unit test to inject mocks

nits:

• "unit tests" (plural)
• missing period :)

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 17 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

I think in Go, it's a convention that imports with a blank identifier need a comment.

Even if not, a comment is useful here to educate the reader why we're doing this.

copy comment from G3 code which has the same import.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 52 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

mctx

Please use ctx here. The manager Context (mctx) is used internally by the Manager and controls its lifetime. If the context is done, the manager should shut down and stop the workflow.

On the other hand, Create() and Start() below use the passed in context only for operations on the topology i.e. it's a different usage. Therefore, let's use ctx. If mctx should be used, there shouldn't be a parameter for a Context in the first place.

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 64 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Please remove debug statements.

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 66 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

mctx

ctx

Not used anymore, using _.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 67 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

testworkflow

resharding workflow

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 71 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

mctx

ctx

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 72 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Please check the return value and fail the test if != nil.

Done.

---

go/vt/workflow/resharding/horizontal_resharding_workflow_test.go, line 91 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

ctx context.Context,

unused, please remove

Done.

---

go/vt/workflow/resharding/parallel_runner_test.go, line 24 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

ctx

Same comment as in the other file. Please don't use the Manager context for the calls below and instead use context.Background().

Done.

---

go/vt/workflow/resharding/parallel_runner_test.go, line 41 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

t.Fatalf("verify all tasks done failed: %v", err)

FYI:

t.Fatal(err) would be good enough here. The test failure gives you the exact code location and should already have enough error details from the return value.

Can you please change this throughout the file?

Done.

---

Comments from Reviewable

[mberlin-bot]

---

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[wangyipei01]

@alainjobart can you review the [last commit]?(626eea1)
Based on the go readability reviewer's suggestion, I changed 2 method names in manager.go and you are the file owner.

[alainjobart]

LGTM