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.

Sign up for GitHub

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

Jump to bottom

Admin CLI: add describeHistoryHost #826

Merged
merged 8 commits into from
Jun 8, 2018
Merged

Admin CLI: add describeHistoryHost #826

merged 8 commits into from
Jun 8, 2018

Conversation

longquanzheng
Copy link
Contributor

Tested in local laptop:

longer@~/gocode/src/github.com/uber/cadence:(admin-desc-shard)$ ./cadence admin hist desc -ad 127.0.0.1:7934
{
  "numberOfShards": 4,
  "domainCache": {
    "numOfItemsInCacheByID": 1,
    "numOfItemsInCacheByName": 1
  },
  "shardControllerStatus": "started,not stopped,not stopping,",
  "address": "127.0.0.1:7934"
}
longer@~/gocode/src/github.com/uber/cadence:(admin-desc-shard)$
longer@~/gocode/src/github.com/uber/cadence:(admin-desc-shard)$
longer@~/gocode/src/github.com/uber/cadence:(admin-desc-shard)$ ./cadence admin hist desc -ad 127.0.0.1:7934 -pf
{
  "numberOfShards": 4,
  "shardIDs": [
    0,
    1,
    2,
    3
  ],
  "domainCache": {
    "numOfItemsInCacheByID": 1,
    "numOfItemsInCacheByName": 1
  },
  "shardControllerStatus": "started,not stopped,not stopping,",
  "address": "127.0.0.1:7934"
}
longer@~/gocode/src/github.com/uber/cadence:(admin-desc-shard)$ ./cadence admin hist desc -sid 1
{
  "numberOfShards": 4,
  "domainCache": {
    "numOfItemsInCacheByID": 1,
    "numOfItemsInCacheByName": 1
  },
  "shardControllerStatus": "started,not stopped,not stopping,",
  "address": "127.0.0.1:7934"
}
longer@~/gocode/src/github.com/uber/cadence:(admin-desc-shard)$ ./cadence admin hist desc -w helloworld_1efeb0df-5d70-4aaa-b6b7-ac178ebb3066
{
  "numberOfShards": 4,
  "domainCache": {
    "numOfItemsInCacheByID": 1,
    "numOfItemsInCacheByName": 1
  },
  "shardControllerStatus": "started,not stopped,not stopping,",
  "address": "127.0.0.1:7934"
}

vancexu
vancexu suggested changes Jun 7, 2018
View reviewed changes
idl/github.com/uber/cadence/admin.thrift Outdated
50: optional string address
}

struct DomainCache{
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more precise to be named DomainCacheInfo or something else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good to me.

tools/cli/adminCommands.go Outdated

wid := c.String(FlagWorkflowID)
sid := c.String(FlagShardID)
addr := c.String(FlagAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

You need a new flag FlagHistoryAddress here to avoid collision of global frontend address and this specific history address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a good point. I started with different flag but then found a similar one and tried to reuse it. But that won't work because of the conflict.

tools/cli/admin.go Outdated
Name: FlagAddressWithAlias,
Usage: "Host address(IP:PORT)",
},
// We have to use string type here because the CLI framework will mix 0 and empty value
Copy link
Contributor

Choose a reason for hiding this comment

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

You could still use IntFlag here. To tell difference between 0 and empty, use c.IsSet().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

tools/cli/adminCommands.go Outdated
printFully := c.Bool(FlagPrintFullyDetail)

if len(wid) <= 0 && len(sid) <= 0 && len(addr) <= 0 {
ExitIfError(fmt.Errorf("at least one of them is required to provide to lookup host: workflowID, shardID and host address"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's deprecated ExitIfError and use ErrorAndExit. The former one had a confusing STACK_TRACE thing and lack of color and human readable print.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

idl/github.com/uber/cadence/admin.thrift Outdated
@@ -49,3 +59,23 @@ struct DescribeWorkflowExecutionResponse{
40: optional string mutableStateInCache
50: optional string mutableStateInDatabase
}

//At least one of the parameters needs to be provided
struct DescribeHistoryHostRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we put the request and response struct to shared.thrift, to avoid redundancy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good to me.

vancexu
vancexu approved these changes Jun 8, 2018
View reviewed changes
Copy link
Contributor

@vancexu vancexu left a comment

Choose a reason for hiding this comment

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

LGTM after address 2 more small comments.

idl/github.com/uber/cadence/admin.thrift Outdated
@@ -36,6 +36,16 @@ service AdminService {
3: shared.EntityNotExistsError entityNotExistError,
4: shared.AccessDeniedError accessDeniedError,
)

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

The indent looks not good.

service/history/handler.go Outdated
status += "not stopped,"
}
if h.controller.isStopping {
status += "stopping,"
Copy link
Contributor

Choose a reason for hiding this comment

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

The comma at the end of string looks not necessary, or is that on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not on purpose. I will remove it.

@longquanzheng longquanzheng merged commit 8441b49 into cadence-workflow:master Jun 8, 2018
@longquanzheng longquanzheng deleted the admin-desc-shard branch January 5, 2020 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vancexu vancexu vancexu approved these changes

@yiminc-zz yiminc-zz Awaiting requested review from yiminc-zz

@samarabbas samarabbas Awaiting requested review from samarabbas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@longquanzheng @vancexu