Skip to content
This repository has been archived by the owner on Feb 21, 2024. It is now read-only.

Implement 'config' CLI command. #541

Merged
merged 1 commit into from May 22, 2017

Conversation

benbjohnson
Copy link
Contributor

Overview

Renames config to generate-config and implements a new config command that generates the configuration file based on the current state instead of printing a static string.

Fixes #375, #366.

)

var Conf *ctl.ConfigCommand

func NewConfigCommand(stdin io.Reader, stdout, stderr io.Writer) *cobra.Command {
Conf = ctl.NewConfigCommand(os.Stdin, os.Stdout, os.Stderr)
Server := server.NewCommand(stdin, stdout, stderr)
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't show current config as I ran pilosa config, correct me if I'm wrong but does this line always create new server with default config, so Server.Config isn't updated with current config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that line starts with the default configuration but it gets passed to BuildServerFlags() which attaches to cobra and updates the default configuration based on a --config file passed in, command line flags, etc.

For example, I wrote out a /tmp/pilosa.conf:

data-dir = "~/.pilosa2"
bind = "localhost:10102"
max-writes-per-request = 5002

[cluster]
  poll-interval = "3m0s"
  replicas = 2
  hosts = [
    "localhost:10102",
    "localhost:10103",
  ]

[anti-entropy]
  interval = "20m0s"

[profile]
  cpu = "2"
  cpu-time = "40s"

[plugins]
  path = "test"

And then passed it to the config command it (while overriding the --data-dir):

$ pilosa --config /tmp/pilosa.conf config --data-dir "/foo/bar"
data-dir = "/foo/bar"
host = "localhost:10102"
log-path = ""
max-writes-per-request = 5002

[anti-entropy]
  interval = 1200000000000

[cluster]
  gossip-seed = ""
  hosts = ["localhost:10102","localhost:10103"]
  internal-hosts = []
  internal-port = ""
  polling-interval = 180000000000
  replicas = 2
  type = "static"

[plugins]
  path = "test"

Copy link
Contributor

Choose a reason for hiding this comment

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

That what I understand it's supposed to do. However I'm not able to get the config passed in, instead pilosa config printed out the default config. When I printed out config passed in BuildServerFlags(), it's actually the default config.

&{DataDir: Host:localhost:10101 Cluster:{ReplicaN:1 Type:static Hosts:[] InternalHosts:[] PollingInterval:1m0s InternalPort: GossipSeed:} Plugins:{Path:} AntiEntropy:{Interval:10m0s} MaxWritesPerRequest:5000 LogPath:}

Here what's I did, please let me know if any steps that I missed:

my current config as following, which I ran with command pilosa server --config ~/Documents/node1.conf

data-dir = "/tmp/pil5_0"
bind = "127.0.0.1:10101"

[cluster]
  poll-interval = "2m0s"
  replicas = 2
  partitions = 128
  hosts = [
    "127.0.0.1:10101",
    "127.0.0.1:15001",
  ]
type = "gossip"
  internal-port = 14000
  gossip-seed = "127.0.0.1:14000"
  internal-hosts = [
    "127.0.0.1:14000",
    "127.0.0.1:14001",
]

[anti-entropy]
  interval = "1m0s"

[metric]
  service = "statsd"
  host = "127.0.0.1:8125"
  poll-interval = "0m15s"

On another iterm tab, I ran pilosa config and it printed out:

data-dir = "~/.pilosa"
host = ":10101"
log-path = ""
max-writes-per-request = 5000

[anti-entropy]
  interval = 600000000000

[cluster]
  gossip-seed = ""
  hosts = []
  internal-hosts = []
  internal-port = ""
  polling-interval = 60000000000
  replicas = 1
  type = "static"

[plugins]
  path = ""

@travisturner
Copy link
Member

@codysoyland was the work done here your intention with #375?
it seems like you weren't expecting pilosa config to take any arguments (other than host i suppose?), but at that point it becomes a client to a running pilosa server. I think that's causing confusion with @linhvo as well.

@coveralls
Copy link

coveralls commented May 18, 2017

Coverage Status

Coverage increased (+0.01%) to 66.714% when pulling 3960345 on benbjohnson:375-config into 4a14e38 on pilosa:master.

@codysoyland
Copy link
Contributor

The way that @benbjohnson implemented this is the way I expected it to work. It seems that there is a misunderstanding over intended functionality. If I understand correctly, @linhvo expects that pilosa config (with no arguments) will connect to a running pilosa server and fetch and print out the configuration (correct me if I'm wrong). The reason it prints out the default configuration is because it's not connecting to the existing server and it's not started with the --config flag to tell it what config file to use as a base. @benbjohnson's example with flags works for me: pilosa --config /tmp/pilosa.conf config --data-dir "/foo/bar". We just need to document this clearly. I can see the value in having a client be able to fetch the config from the server, but that would be a separate project.

To summarize:

  • Behavior Update import path #1: pilosa [--config $file] config --arguments takes the same arguments as pilosa server and prints the "final" configuration values.
  • Behavior Remove commented code #2: pilosa config connects to existing pilosa server as a client, fetches the config values, and prints them.

Behavior #1 is my preference.

I did notice a small bug in this though -- the time values produced by pilosa config are large integers without units. Unsure the reason for this. If I write it out to a file and try to use it, it results in Error: time: missing unit in duration 600000000000.

@linhvo
Copy link
Contributor

linhvo commented May 19, 2017

@codysoyland: Oh I see, I expected behavior number 2 as you mentioned, as I understand from reading ticket #375. Sorry if I missed read it. If behavior 1 is expected then what Ben implemented work then

Renames `config` to `generate-config` and implements a new `config`
command that generates the configuration file based on the current
state instead of printing a static string.
@benbjohnson
Copy link
Contributor Author

@codysoyland Fixed the duration encoding in 788386b. It looks like pelletier/go-toml has its own Marshaler interface. https://godoc.org/github.com/pelletier/go-toml#Marshaler

@linhvo Let me know if there's anything else that needs to be fixed.

@coveralls
Copy link

coveralls commented May 22, 2017

Coverage Status

Coverage decreased (-0.007%) to 66.704% when pulling 788386b on benbjohnson:375-config into 7693fc4 on pilosa:master.

@benbjohnson benbjohnson merged commit b9c460d into FeatureBaseDB:master May 22, 2017
@benbjohnson benbjohnson deleted the 375-config branch May 22, 2017 16:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants