Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add command cluster create-failure-domain #21

Merged
merged 18 commits into from
Sep 3, 2019

Conversation

zymap
Copy link
Member

@zymap zymap commented Aug 30, 2019

Master issue: #2

 USED FOR:
    This command is used for creating a failure domain of the <cluster-name>.

REQUIRED PERMISSION:
    This command requires super-user permissions.

EXAMPLES:
    #creating the failure domain
    pulsarctl clusters create-failure-domain <cluster-name> <domain-name>

    #creating the failure domain with brokers
    pulsarctl clusters create-failure-domain --broker-list <cluster-A> --broker-list <cluster-B> <cluster-name> <domain-name>

OUTPUT:
    #normal output
    Create failure domain <domain-name> for cluster <cluster-name> succeed

    #the args need to be specified as <cluster-name> <domain-name>
    [✖]  need specified two names for cluster and failure domain

    #the specified cluster does not exist in the broker
    [✖]  code: 404 reason: Cluster does not exist

Usage: pulsarctl clusters create-failure-domain [flags]

Aliases: create-failure-domain, cfd

FailureDomainData flags:
  -b, --broker-list strings   Set the failure domain clusters

Common flags:
  -s, --admin-service-url string    The admin web service url that pulsarctl connects to. (default "http://localhost:8080")
      --auth-params string          Authentication parameters are used to configure the public and private key files required by tls
                                     For example: "tlsCertFile:val1,tlsKeyFile:val2"
  -C, --color string                toggle colorized logs (true,false,fabulous) (default "true")
  -h, --help                        help for this command
      --tls-allow-insecure          Allow TLS insecure connection
      --tls-trust-cert-pat string   Allow TLS trust cert file path
  -v, --verbose int                 set log level, use 0 to silence, 4 for debugging (default 3)

Use 'pulsarctl clusters create-failure-domain [command] --help' for more information about a command.

@zymap zymap self-assigned this Aug 30, 2019
@zymap zymap added this to the 0.0.1 milestone Aug 30, 2019
@zymap zymap changed the title [WIP] Clusters create failure domain [WIP] Add command cluster create-failure-domain Aug 30, 2019
@zymap zymap force-pushed the clusters_create_failure_domain branch 2 times, most recently from 9902b67 to d230065 Compare September 3, 2019 02:43
@zymap zymap requested a review from sijie September 3, 2019 02:46
@zymap zymap changed the title [WIP] Add command cluster create-failure-domain Add command cluster create-failure-domain Sep 3, 2019
@zymap zymap requested review from wolfstudy and removed request for sijie September 3, 2019 02:46
@zymap zymap removed their assignment Sep 3, 2019
@zymap zymap requested a review from sijie September 3, 2019 02:46
@zymap zymap self-assigned this Sep 3, 2019
@zymap zymap force-pushed the clusters_create_failure_domain branch from 386fda2 to 919d4d0 Compare September 3, 2019 03:56
@sijie
Copy link
Member

sijie commented Sep 3, 2019

The output doesn't seem to be correct to me.

    #normao output
    Create failure domain <domain-name> for cluster <cluster-name> succeed

    #output of doesn't specified a cluster name
    [✖]  only one argument is allowed to be used as a name

    #output of cluster doesn't exist
    [✖]  code: 404 reason: Cluster does not exist

@zymap
Copy link
Member Author

zymap commented Sep 3, 2019

@sijie Ah. It already fixes in the code. I forget fix the description of this pull request.

pkg/ctl/cluster/create_failure_domain.go Outdated Show resolved Hide resolved
pkg/ctl/cluster/create_failure_domain.go Outdated Show resolved Hide resolved
pkg/ctl/cluster/create_failure_domain.go Outdated Show resolved Hide resolved
}
out = append(out, successOut)
out = append(out, argsError)
out = append(out, clusterNonExist)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clusterNonExist is for cluster commands. please make sure the error message is correct.

pkg/ctl/cluster/create_failure_domain.go Outdated Show resolved Hide resolved
wolfstudy and others added 2 commits September 3, 2019 13:38
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
@zymap
Copy link
Member Author

zymap commented Sep 3, 2019

@sijie PTAL

"testing"
)

func TestCreateFailureDomainCmdSuccess(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add test cases for failure cases.

desc.CommandPermission = "This command requires super-user permissions."

var examples []pulsar.Example
create := pulsar.Example{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does Pulsar allow creating a failure domain without a broker list?

I don't think that is a valid case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. I will check the broker list in pulsarctl.

Using pulsar-admin

bin/pulsar-admin clusters create-failure-domain --domain-name hello standalone

bin/pulsar-admin clusters list-failure-domains standalone
{
  "hello" : {
    "brokers" : [ ]
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

### Motivation

We can't get the error info when running a command. When testing a command, we need to verify the error output.

### Modification

Make a handler to handle errors. Create a test handler in TestClusterCommand for test other commands.
@sijie sijie merged commit 0d9d8c3 into master Sep 3, 2019
@sijie sijie deleted the clusters_create_failure_domain branch September 3, 2019 19:05
@sijie sijie mentioned this pull request Sep 4, 2019
29 tasks
tisonkun pushed a commit to tisonkun/pulsar-client-go that referenced this pull request Aug 15, 2023
Master issue: streamnative/pulsarctl#2 

```
 USED FOR:
    This command is used for creating a failure domain of the <cluster-name>.

REQUIRED PERMISSION:
    This command requires super-user permissions.

EXAMPLES:
    #creating the failure domain
    pulsarctl clusters create-failure-domain <cluster-name> <domain-name>

    #creating the failure domain with brokers
    pulsarctl clusters create-failure-domain --broker-list <cluster-A> --broker-list <cluster-B> <cluster-name> <domain-name>

OUTPUT:
    #normal output
    Create failure domain <domain-name> for cluster <cluster-name> succeed

    #the args need to be specified as <cluster-name> <domain-name>
    [✖]  need specified two names for cluster and failure domain

    #the specified cluster does not exist in the broker
    [✖]  code: 404 reason: Cluster does not exist

Usage: pulsarctl clusters create-failure-domain [flags]

Aliases: create-failure-domain, cfd

FailureDomainData flags:
  -b, --broker-list strings   Set the failure domain clusters

Common flags:
  -s, --admin-service-url string    The admin web service url that pulsarctl connects to. (default "http://localhost:8080")
      --auth-params string          Authentication parameters are used to configure the public and private key files required by tls
                                     For example: "tlsCertFile:val1,tlsKeyFile:val2"
  -C, --color string                toggle colorized logs (true,false,fabulous) (default "true")
  -h, --help                        help for this command
      --tls-allow-insecure          Allow TLS insecure connection
      --tls-trust-cert-pat string   Allow TLS trust cert file path
  -v, --verbose int                 set log level, use 0 to silence, 4 for debugging (default 3)

Use 'pulsarctl clusters create-failure-domain [command] --help' for more information about a command.
```
tisonkun pushed a commit to apache/pulsar-client-go that referenced this pull request Aug 16, 2023
Master issue: streamnative/pulsarctl#2 

```
 USED FOR:
    This command is used for creating a failure domain of the <cluster-name>.

REQUIRED PERMISSION:
    This command requires super-user permissions.

EXAMPLES:
    #creating the failure domain
    pulsarctl clusters create-failure-domain <cluster-name> <domain-name>

    #creating the failure domain with brokers
    pulsarctl clusters create-failure-domain --broker-list <cluster-A> --broker-list <cluster-B> <cluster-name> <domain-name>

OUTPUT:
    #normal output
    Create failure domain <domain-name> for cluster <cluster-name> succeed

    #the args need to be specified as <cluster-name> <domain-name>
    [✖]  need specified two names for cluster and failure domain

    #the specified cluster does not exist in the broker
    [✖]  code: 404 reason: Cluster does not exist

Usage: pulsarctl clusters create-failure-domain [flags]

Aliases: create-failure-domain, cfd

FailureDomainData flags:
  -b, --broker-list strings   Set the failure domain clusters

Common flags:
  -s, --admin-service-url string    The admin web service url that pulsarctl connects to. (default "http://localhost:8080")
      --auth-params string          Authentication parameters are used to configure the public and private key files required by tls
                                     For example: "tlsCertFile:val1,tlsKeyFile:val2"
  -C, --color string                toggle colorized logs (true,false,fabulous) (default "true")
  -h, --help                        help for this command
      --tls-allow-insecure          Allow TLS insecure connection
      --tls-trust-cert-pat string   Allow TLS trust cert file path
  -v, --verbose int                 set log level, use 0 to silence, 4 for debugging (default 3)

Use 'pulsarctl clusters create-failure-domain [command] --help' for more information about a command.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants