KC-1368 & KC-1372: Support cluster linking Link/Mirror operation #791 by ctan888 · Pull Request #793 · confluentinc/cli

Cheng (ctan888) · 2021-02-22T18:54:37Z

Adapting the existing "link command" to Rest proxy v3. Will fall back to the old Kafka Api path if rest proxy is not available.
Create a new "mirror command" which will use rest proxy v3. The old "mirror" related operation in "topic command" will remain the same behavior and will be deprecated in the future
Unit test for mirror/link command. (Not sure we need the golden file tests, since we will have cc system test covering in the near future)

Miles Todzo (mtodzo)

Few minor comments, but mostly looks good!

I'm requesting changes for the testing though (repasting comment here):

Right now these unit tests are only verifying that no error is occurring - I'd ask that we have the tests also doing one of the following or both:

add the expected request data into the expect chan (see ListTopicTest in kafka/command_test.go), and then in the mock/kafka_rest.go compare the expected data against the actual request data to make sure they are equal (see CreateACLs and assertEqualBindings in mock/kafka.go)
Change the command output to be buffer that we can verify the contains the correct output (see TestShouldNotConfigureOnDryRun in ksql/command_test.go) and check to see that the proper output is being printed.

Also, it would be great to add some integration tests (the ones with the .golden files) since sometimes cc-system tests is broken for a non-CLI reason and this way we can ensure there's still some e-2-e testing. These are quick to add if you follow the ones in test/kafka_test.go that use the env var XX_CCLOUD_USE_KAFKA_REST.

Cheng (ctan888) · 2021-02-26T19:35:57Z

Miles Todzo (@mtodzo) Thanks for the review. I've addressed all the comments and added the new set of tests following your suggestion. Could you take another look and let me know if there's anything else to change? Thanks.

Miles Todzo (mtodzo) · 2021-02-26T20:51:51Z

 	expect := make(chan interface{})
 	for _, link := range Links {
-		cmd := newCmd(expect, false)
+		cmd := newCmd(expect, true)


should there be tests for both newCmd(expect, true) and newCmd(expect, false) so that link commands are tested using rest proxy AND using kafka api?

I don't think we need tests for kafka-api in cluster linking, as the rest proxy is guaranteed to be enabled for all cluster linking clusters.

Cheng (ctan888) · 2021-02-27T10:24:43Z

Hi Miles Todzo (@mtodzo) . Thanks for the review again. Sorry for the wrongly pushed branch confusion for the tests.

I've addressed all your new comments. Let me know if we're good to go now.

Miles Todzo (mtodzo)

good to merge after nit comments and sdk update/CI passes

Miles Todzo (mtodzo) · 2021-03-01T20:30:53Z

+	linkName := args[0]
+	kafkaREST, _ := c.GetKafkaREST()
+	if kafkaREST == nil {
+		fmt.Println("rest proxy not available")


remove print statement

Miles Todzo (mtodzo) · 2021-03-01T20:32:22Z

+		utils.Printf(cmd, errors.DeletedLinkMsg, linkName)
 	}

 	return err


can we just do a return nil for clarity

Miles Todzo (mtodzo) · 2021-03-01T20:47:37Z

@@ -1,3 +1,4 @@
+rest proxy not available


from the printed line commented on earlier

Miles Todzo (mtodzo) · 2021-03-01T20:48:43Z

+	rpLinks             = "/kafka/v3/clusters/{cluster}/links"
+	rpLinkConfigs       = "/kafka/v3/clusters/{cluster}/links/{link}/configs"
+	rpMirror            = "/kafka/v3/clusters/{cluster}/links/{link}/mirrors/{destination_topic_name}"
+	rpAllMirrors           = "/kafka/v3/clusters/{cluster}/links/-/mirrors"


if you run make fmt it should fix indents

Miles Todzo (mtodzo) · 2021-03-01T20:49:50Z

+		{args: "kafka link list --cluster lkc-describe-topic", fixture: "kafka/cluster-linking/list-link-plain.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka link list --cluster lkc-describe-topic -o json", fixture: "kafka/cluster-linking/list-link-json.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka link list --cluster lkc-describe-topic -o yaml", fixture: "kafka/cluster-linking/list-link-yaml.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka link describe --cluster lkc-describe-topic link-1", fixture: "kafka/cluster-linking/describe-link-plain.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka link describe --cluster lkc-describe-topic link-1 -o json", fixture: "kafka/cluster-linking/describe-link-json.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka link describe --cluster lkc-describe-topic link-1 -o yaml", fixture: "kafka/cluster-linking/describe-link-yaml.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka link list-configs --cluster lkc-describe-topic link-1", fixture: "kafka/cluster-linking/list-link-configs-plain.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka link list-configs --cluster lkc-describe-topic link-1 -o json", fixture: "kafka/cluster-linking/list-link-configs-json.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka link list-configs --cluster lkc-describe-topic link-1 -o yaml", fixture: "kafka/cluster-linking/list-link-configs-yaml.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+
+		{args: "kafka mirror list --cluster lkc-describe-topic --link-name link-1", fixture: "kafka/cluster-linking/list-mirror.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka mirror list --cluster lkc-describe-topic --link-name link-1 -o json", fixture: "kafka/cluster-linking/list-mirror-json.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka mirror list --cluster lkc-describe-topic --link-name link-1 -o yaml", fixture: "kafka/cluster-linking/list-mirror-yaml.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka mirror list --cluster lkc-describe-topic", fixture: "kafka/cluster-linking/list-all-mirror.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka mirror list --cluster lkc-describe-topic -o json", fixture: "kafka/cluster-linking/list-all-mirror-json.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka mirror list --cluster lkc-describe-topic -o yaml", fixture: "kafka/cluster-linking/list-all-mirror-yaml.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka mirror promote topic1 topic2 --cluster lkc-describe-topic --link-name link-1", fixture: "kafka/cluster-linking/promote-mirror.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka mirror promote topic1 topic2 --cluster lkc-describe-topic --link-name link-1 -o json", fixture: "kafka/cluster-linking/promote-mirror-json.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},
+		{args: "kafka mirror promote topic1 topic2 --cluster lkc-describe-topic --link-name link-1 -o yaml", fixture: "kafka/cluster-linking/promote-mirror-yaml.golden", wantErrCode: 0, useKafka: "lkc-describe-topic", env: []string{"XX_CCLOUD_USE_KAFKA_REST=true"}},


sorry i know they're all under TestKafka right now, but would you might moving these under there own test? TestKafkaMirror TestKafkaLink

Miles Todzo (mtodzo) · 2021-03-01T20:50:15Z

 	return normalized
 }

+func RemoveSpace(s string) string {


when is this used?

Miles Todzo (mtodzo) · 2021-03-01T20:55:49Z

+		outputWriter.AddElement(&alterMirrorWrite{
+			DestinationTopicName: result.DestinationTopicName,
+			ErrorMessage: msg,
+			ErrorCode: code,


is there anything else that should be printed? like something about the actual operation that was done? I'm thinking about the case where it's a success and the user then sees the destination topic, and "" for the error fields. But there's nothing that indicates what the change was? I don't really have an understanding of what the mirroring functionality is so this might not be relevant

Alyssa Huang (ahuang98) · 2021-03-12T07:39:24Z

 	UpdatedLinkMsg = "Updated cluster link \"%s\".\n"

+	// kafka mirror commands
+	RestProxyNotAvailableMsg = "Operation not supported: rest proxy is not available in the destination cluster."


nit: could we make the error message more generic ("Operation not supported: rest proxy is not available") so that it can be used in non-cluster linking contexts? I'm implementing CLI commands for consumer lag, and they won't fall back to using kafka API either. If the rest proxy isn't available, I would also be returning a rest-proxy-not-available error

Alyssa Huang (ahuang98) · 2021-04-12T20:30:06Z


 /*************** TEST setup/helpers ***************/
-func newCmd(expect chan interface{}, enableREST bool) *cobra.Command {
+func newMockCmd(kafkaExpect chan interface{}, kafkaRestExpect chan interface{}, enableREST bool) *cobra.Command {


I'm going to merge in these helper method changes (L985-L1029) so I can use it for my consumer group PR, thank you Cheng!
https://github.com/confluentinc/cli/pull/820/files#r604433061

…mments beginning by #, in the config file parsing process

Cheng (ctan888) added 4 commits February 20, 2021 13:48

Finishe Link

e48ee40

adding files

a52fa37

finish mirror

32d1a93

finish unit tests

9d15a0b

Cheng (ctan888) requested a review from a team as a code owner February 22, 2021 18:54

Cheng (ctan888) changed the title ~~Link nmirror~~ KC-1368 & KC-1372: Support cluster linking Link/Mirror operation #791 Feb 22, 2021

Cheng (ctan888) mentioned this pull request Feb 22, 2021

KC-1368 & KC-1372: Support cluster linking Link/Mirror operation #791

Closed

Miles Todzo (mtodzo) suggested changes Feb 22, 2021

View reviewed changes

Cheng (ctan888) added 2 commits February 25, 2021 23:03

adding tests at the request layer

587d1e1

addressing NIT comments

b3d72db

Miles Todzo (mtodzo) reviewed Feb 26, 2021

View reviewed changes

Comment thread mock/kafka_rest.go Outdated

Miles Todzo (mtodzo) reviewed Feb 26, 2021

View reviewed changes

Comment thread internal/cmd/kafka/command_test.go Outdated

Miles Todzo (mtodzo) reviewed Feb 26, 2021

View reviewed changes

Comment thread internal/cmd/kafka/command_mirror.go Outdated

Comment thread internal/cmd/kafka/command_test.go

Comment thread mock/kafka_rest.go Outdated

Miles Todzo (mtodzo) reviewed Feb 26, 2021

View reviewed changes

Comment thread internal/cmd/kafka/command_mirror.go Outdated

Cheng (ctan888) added 2 commits February 27, 2021 01:48

add golden tests

a630775

revolke string replacement

f3134db

Cheng (ctan888) force-pushed the linkNmirror branch from fb5364a to f3134db Compare February 27, 2021 10:15

remove fmt.printf

461cdcc

Cheng (ctan888) requested a review from Miles Todzo (mtodzo) February 27, 2021 10:24

Cheng (ctan888) force-pushed the linkNmirror branch 2 times, most recently from 43b6cc4 to 55a23ea Compare February 27, 2021 10:36

revoke mis-replacing

caeaed7

Cheng (ctan888) force-pushed the linkNmirror branch from 55a23ea to caeaed7 Compare February 27, 2021 10:40

Cheng (ctan888) added 2 commits February 27, 2021 02:51

revoke kafka_test unnecessary changes

7a1dffc

format

0dd9e05

Cheng (ctan888) force-pushed the linkNmirror branch from b6b4fa3 to 0dd9e05 Compare February 27, 2021 10:55

adapt list mirror api changes

68ed44b

Miles Todzo (mtodzo) approved these changes Mar 1, 2021

View reviewed changes

Cheng (ctan888) added 2 commits March 9, 2021 15:13

adding lag to describe, list, and alter

a06df8f

fix the cli config issue

463f0b0

Alyssa Huang (ahuang98) reviewed Mar 12, 2021

View reviewed changes

Cheng (ctan888) added 3 commits April 2, 2021 14:33

adapt to the api field renaming

be5b80e

fall back create mirror

3ee337b

Added parameter requirement

6a10c90

Alyssa Huang (ahuang98) reviewed Apr 12, 2021

View reviewed changes

David Hyde (DABH) force-pushed the master branch from 7675e6d to 8f086e7 Compare April 16, 2021 02:54

Cheng (ctan888) added 16 commits April 21, 2021 14:51

Changes based on E2E tests

270a528

tag the sdk and update the dependency

56867fa

resolve cofl

62e68ad

update go sum

770cf4d

Improvement: KGLOBAL-410

10b99b5

improve the creat link manual

a480c57

KGLOBAL-407: added --source-api-key and --source-api-secret flags

39e0790

KC-409: change the manual of mirror create

6cb4f3c

KGLOBAL-408: ignoring the leading and ending while spaces, and the co…

3ab8c59

…mments beginning by #, in the config file parsing process

fix test conflicts

1a80685

merge and resolve conflicts/

fd4dd88

fix tests

a8fe146

fix make lint-go

1544dab

supress lint

678c724

fix confl

a5d38dc

resl confl

9825de0

Cheng (ctan888) force-pushed the linkNmirror branch from 7071d22 to 9825de0 Compare May 3, 2021 17:16

Cheng (ctan888) merged commit 6a9cb63 into master May 3, 2021

Cheng (ctan888) deleted the linkNmirror branch May 3, 2021 20:56

Uh oh!

Conversation

Cheng (ctan888) commented Feb 22, 2021

Uh oh!

Miles Todzo (mtodzo) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cheng (ctan888) commented Feb 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cheng (ctan888) commented Feb 27, 2021

Uh oh!

Miles Todzo (mtodzo) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alyssa Huang (ahuang98) Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Alyssa Huang (ahuang98) Mar 12, 2021 •

edited

Loading