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 conn_attrs flag to admin db commands #4226

Merged
merged 1 commit into from
May 28, 2021

Conversation

Shaddoll
Copy link
Member

@Shaddoll Shaddoll commented May 26, 2021

What changed?

  • Introduce StringMap type to parse key=val CLI input into string map
  • Add conn_attrs flag to admin db commands and use it for SQL database connection

Why?
To address #2777. Docstore will use conn_attrs for database connection.

How did you test it?
local test, new unit test

zijian@zijian-C02XW43AJGH6 cli % ./cli adm shard d --conn_attrs "a=b" --shard_id 0 --db_port 9042 --conn_attrs cd --db_type mysql 
Incorrect Usage: invalid value "cd" for flag -conn_attrs: should be in 'key=value' format

NAME:
   cadence admin shard describe - Describe shard by Id

USAGE:
   cadence admin shard describe [command options] [arguments...]

OPTIONS:
   --db_type value                 persistence type. Current supported options are [mysql postgres cassandra] (default: "cassandra")
   --db_address value              persistence address (right now only cassandra is fully supported) (default: "127.0.0.1")
   --db_port value                 persistence port (default: 9042)
   --db_region value               persistence region
   --username value                persistence username
   --password value                persistence password
   --keyspace value                cassandra keyspace (default: "cadence")
   --db_name value                 sql database name (default: "cadence")
   --encoding_type value           sql database encoding type (default: "thriftrw")
   --decoding_types value          sql database decoding types (default: "thriftrw")
   --tls                           enable TLS over cassandra connection
   --tls_cert_path value           cassandra tls client cert path (tls must be enabled)
   --tls_key_path value            cassandra tls client key path (tls must be enabled)
   --tls_ca_path value             cassandra tls client ca path (tls must be enabled)
   --tls_enable_host_verification  cassandra tls verify hostname and server cert (tls must be enabled)
   --conn_attrs value              a key-value set of sql database connection attributes (must be in key=value format) (default: map[a:b])
   --shard_id value                The Id of the shard to describe (default: 0)
   
invalid value "cd" for flag -conn_attrs: should be in 'key=value' format

Potential risks

Release notes

Documentation Changes

@coveralls
Copy link

coveralls commented May 26, 2021

Pull Request Test Coverage Report for Build ad5ffe24-baec-4ba6-a5dd-dd215dc17b41

  • 18 of 37 (48.65%) changed or added relevant lines in 3 files are covered.
  • 47 unchanged lines in 9 files lost coverage.
  • Overall coverage increased (+0.02%) to 60.288%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tools/cli/adminCommands.go 0 8 0.0%
tools/common/flag/flag.go 13 24 54.17%
Files with Coverage Reduction New Missed Lines %
client/history/client.go 2 44.78%
client/history/metricClient.go 2 49.43%
common/task/weightedRoundRobinTaskScheduler.go 2 89.12%
service/history/handler.go 2 46.67%
service/history/queue/timer_queue_processor.go 2 58.77%
common/cache/lru.go 3 90.73%
service/frontend/workflowHandler.go 4 58.35%
service/history/historyEngine.go 9 71.18%
common/persistence/cassandra/cassandraPersistence.go 21 55.47%
Totals Coverage Status
Change from base Build 8877bb7e-1e22-4212-9252-ad05b7afefd5: 0.02%
Covered Lines: 88600
Relevant Lines: 146962

💛 - Coveralls

Copy link
Contributor

@longquanzheng longquanzheng left a comment

Choose a reason for hiding this comment

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

Just minor comments

@Shaddoll Shaddoll force-pushed the admin-flag branch 3 times, most recently from f8f032e to ca7b7e0 Compare May 28, 2021 22:49
@longquanzheng
Copy link
Contributor

LGTM

@Shaddoll Shaddoll merged commit 932303f into cadence-workflow:master May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants