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 multi local namespace support #1335

Merged
merged 7 commits into from
Jan 31, 2019
Merged

Conversation

benraskin92
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Jan 31, 2019

Codecov Report

Merging #1335 into master will increase coverage by <.1%.
The diff coverage is 34.7%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1335     +/-   ##
========================================
+ Coverage    70.6%   70.7%   +<.1%     
========================================
  Files         823     822      -1     
  Lines       70217   70179     -38     
========================================
+ Hits        49599   49624     +25     
+ Misses      17384   17323     -61     
+ Partials     3234    3232      -2
Flag Coverage Δ
#aggregator 81.6% <ø> (ø) ⬆️
#cluster 85.6% <ø> (ø) ⬆️
#collector 78.4% <ø> (ø) ⬆️
#dbnode 80.8% <ø> (+0.1%) ⬆️
#m3em 73.2% <ø> (ø) ⬆️
#m3ninx 74.2% <ø> (ø) ⬆️
#m3nsch 51.1% <ø> (ø) ⬆️
#metrics 17.8% <ø> (ø) ⬆️
#msg 74.9% <ø> (ø) ⬆️
#query 64.3% <34.7%> (+0.1%) ⬆️
#x 76.2% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 806e93b...59dbb99. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 31, 2019

Codecov Report

Merging #1335 into master will increase coverage by 0.1%.
The diff coverage is 34.7%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1335     +/-   ##
========================================
+ Coverage    70.6%   70.7%   +0.1%     
========================================
  Files         823     822      -1     
  Lines       70217   70179     -38     
========================================
+ Hits        49599   49645     +46     
+ Misses      17384   17304     -80     
+ Partials     3234    3230      -4
Flag Coverage Δ
#aggregator 81.6% <ø> (ø) ⬆️
#cluster 85.6% <ø> (ø) ⬆️
#collector 78.4% <ø> (ø) ⬆️
#dbnode 80.9% <ø> (+0.2%) ⬆️
#m3em 73.2% <ø> (ø) ⬆️
#m3ninx 74.2% <ø> (ø) ⬆️
#m3nsch 51.1% <ø> (ø) ⬆️
#metrics 17.8% <ø> (ø) ⬆️
#msg 74.9% <ø> (ø) ⬆️
#query 64.3% <34.7%> (+0.1%) ⬆️
#x 76.2% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 806e93b...81f03e0. Read the comment docs.

@@ -16,9 +16,11 @@ directory on your host for durability:

```
docker pull quay.io/m3/m3dbnode:latest
docker run -p 7201:7201 -p 9003:9003 --name m3db -v $(pwd)/m3db_data:/var/lib/m3db quay.io/m3/m3dbnode:latest
docker run -p 7201:7201 -p 7203:7203 -p 9003:9003 --name m3db -v $(pwd)/m3db_data:/var/lib/m3db -v $GOPATH/src/github.com/m3db/m3/src/dbnode/config/m3dbnode-local-etcd.yml:/etc/m3dbnode/m3dbnode.yml quay.io/m3/m3dbnode:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a little comment about what each port being exposed does?

Copy link
Contributor

Choose a reason for hiding this comment

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

yo so this command now includes a $GOPATH which means it will only work for users who have already cloned . the m3 repository into their $GOPATH.

I think instead you should link to the config file in Github and tell them to copy and paste that into a file locally or something

Copy link
Contributor

Choose a reason for hiding this comment

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

And then add something like: "note that this configuration file configuration for both coordinator AND M3DB because its running as a single binary, if you were to run these applications separately then they would have separate configuration.

I know it is kind of saying the same thing over and over again but it it pretty confusing stuff

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like the idea of just having a command you can copy and paste into your terminal - I'll make a note though if you don't have a GOPATH setup

Copy link
Contributor

Choose a reason for hiding this comment

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

I really think you should remove the $GOPATH. Copy paste and working is fine but "copy and paste this config file into your local directory" is way easier than:

1. Install golang
2. Setup your $GOPATH
3. Clone the M3 repository and make sure you clone it into $GOPATH/src/....

```

**Note:** This setup runs `M3DB` and `M3Coordinator` as one application and should only be used for testing/development purposes. If you want to run a clustered `M3DB` with a separate `M3Coordinator`, please [see here](cluster_hard_way.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

"with a separate . M3Coordinator process (which is our recommended production setup)...`

```

**Note:** This setup runs `M3DB` and `M3Coordinator` as one application and should only be used for testing/development purposes. If you want to run a clustered `M3DB` with a separate `M3Coordinator`, please [see here](cluster_hard_way.md).

<!-- TODO: link to docs containing explanations of what namespaces, the coordinator,
Copy link
Contributor

Choose a reason for hiding this comment

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

we have explanatory docs for namespaces/placement under operational guides if you want to link to them

@@ -37,6 +39,20 @@ curl -X POST http://localhost:7201/api/v1/database/create -d '{
}'
```

**Note:** If you want to create more than one namespace, you should follow the [instructions here](../operational_guide/namespace_configuration.md) and also add the namespace you created to the `local` section of the `m3dbnode-local-etcd.yml` file used in the `docker run` command above with the appropriate aggregation options specified. For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

"with the appropriate aggregation options specified" could you add: "for more information on our aggregation functionality, check out our M3Query documentation"

@@ -92,3 +92,11 @@ remote_read:
remote_write:
- url: "http://localhost:7201/api/v1/prom/remote/write"
```

Also, if you want to see metrics generated by the `m3coordinator` or `m3query` process such as number of queries, etc., you should add the following job under `scrape_configs`:
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

"Also, we recommend adding M3DB and M3Coordinator/`M3Query' to your list of jobs so that you can monitor them with Prometheus. With Prometheus scraping setup, you can also use our pre-configured M3DB Grafana dashboard"

```json
- job_name: 'query'
static_configs:
- targets: ['localhost:7203']
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a scrape job for M3DB too?

@@ -117,6 +117,11 @@ data:
listenAddress:
type: "config"
value: "0.0.0.0:7201"
local:
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct for kube?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah - @schallert said to update here as well

@@ -3,6 +3,12 @@ coordinator:
type: "config"
value: "0.0.0.0:7201"

local:
Copy link
Contributor

Choose a reason for hiding this comment

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

hm...I don't think this is needed. m3_stack runs m3coordinator as an external binary, so I don't . think you need to set this here. To be honest I'm not sure why the coordinator config is here at all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm I'll leave it here for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

but its not even the correct list of namespaces, m3_stack configures a bunch of other namespaces

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So you think I should remove?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so, it'll be less confusing. We should probably remove the coordinator config entirely from this file but its a bigger change.

@@ -3,6 +3,12 @@ coordinator:
type: "config"
value: "0.0.0.0:7201"

# local:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this commented out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was testing - will uncomment.

Copy link
Contributor

@richardartoul richardartoul left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -16,9 +16,11 @@ directory on your host for durability:

```
docker pull quay.io/m3/m3dbnode:latest
docker run -p 7201:7201 -p 9003:9003 --name m3db -v $(pwd)/m3db_data:/var/lib/m3db quay.io/m3/m3dbnode:latest
docker run -p 7201:7201 -p 7203:7203 -p 9003:9003 --name m3db -v $(pwd)/m3db_data:/var/lib/m3db -v $GOPATH/src/github.com/m3db/m3/src/dbnode/config/m3dbnode-local-etcd.yml:/etc/m3dbnode/m3dbnode.yml quay.io/m3/m3dbnode:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

I really think you should remove the $GOPATH. Copy paste and working is fine but "copy and paste this config file into your local directory" is way easier than:

1. Install golang
2. Setup your $GOPATH
3. Clone the M3 repository and make sure you clone it into $GOPATH/src/....

@@ -3,6 +3,12 @@ coordinator:
type: "config"
value: "0.0.0.0:7201"

local:
Copy link
Contributor

Choose a reason for hiding this comment

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

but its not even the correct list of namespaces, m3_stack configures a bunch of other namespaces

@benraskin92 benraskin92 merged commit e80adde into master Jan 31, 2019
@justinjc justinjc deleted the braskin/multi_local_namespace branch June 17, 2019 21:56
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.

2 participants