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

Improve /database/create API #1350

Merged
merged 36 commits into from
Feb 6, 2019
Merged

Improve /database/create API #1350

merged 36 commits into from
Feb 6, 2019

Conversation

richardartoul
Copy link
Contributor

@richardartoul richardartoul commented Feb 3, 2019

This P.R refactors the /database/create API to make it more general purpose, user-friendly, and have better error messages.

The API was originally designed to only be used once to create both a placement and a single namespace, and then never used again. Now that many of our users are creating multiple namespaces, compounded with the fact that configuring a reasonable blocksize for most namespaces is confusing, we want to push users to use this API by default for creating all their namespaces.

This P.R modifies the API to allow it to be called multiple times. If a placement does not exist, it will create it, otherwise it will just initialize the namespace. This logic has a lot of specific error handling to make it user-friendly which is documented in code comments in the P.R.

In addition to modifying the API, the documentation is also updated to explain the new API and push people towards using it.

@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

Merging #1350 into master will increase coverage by <.1%.
The diff coverage is 68.7%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1350     +/-   ##
========================================
+ Coverage    70.7%   70.7%   +<.1%     
========================================
  Files         822     822             
  Lines       70204   70275     +71     
========================================
+ Hits        49648   49715     +67     
  Misses      17325   17325             
- Partials     3231    3235      +4
Flag Coverage Δ
#aggregator 82.3% <ø> (-0.1%) ⬇️
#cluster 85.6% <ø> (ø) ⬆️
#collector 78.4% <ø> (ø) ⬆️
#dbnode 80.8% <ø> (ø) ⬆️
#m3em 73.2% <ø> (ø) ⬆️
#m3ninx 74.3% <ø> (ø) ⬆️
#m3nsch 51.1% <ø> (ø) ⬆️
#metrics 17.8% <ø> (ø) ⬆️
#msg 74.9% <ø> (ø) ⬆️
#query 64.2% <68.7%> (ø) ⬆️
#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 a0cf5b5...4d10a86. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

Merging #1350 into master will increase coverage by <.1%.
The diff coverage is 69.9%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1350     +/-   ##
========================================
+ Coverage    70.7%   70.7%   +<.1%     
========================================
  Files         822     822             
  Lines       70204   70289     +85     
========================================
+ Hits        49653   49737     +84     
+ Misses      17322   17313      -9     
- Partials     3229    3239     +10
Flag Coverage Δ
#aggregator 82.3% <ø> (ø) ⬆️
#cluster 85.6% <ø> (ø) ⬆️
#collector 78.4% <ø> (ø) ⬆️
#dbnode 80.8% <ø> (ø) ⬆️
#m3em 73.2% <ø> (ø) ⬆️
#m3ninx 74.2% <ø> (-0.1%) ⬇️
#m3nsch 51.1% <ø> (ø) ⬆️
#metrics 17.8% <ø> (ø) ⬆️
#msg 74.9% <ø> (ø) ⬆️
#query 64.2% <70%> (ø) ⬆️
#x 76% <66.6%> (-0.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 a0cf5b5...8e58663. Read the comment docs.

@richardartoul richardartoul changed the title [WIP] - Improve /database/create API Improve /database/create API Feb 4, 2019
docs/operational_guide/namespace_configuration.md Outdated Show resolved Hide resolved
retention: <new_retention>
resolution: <new_resolution>
```
Note that the `api/v1/database/create` endpoint is abstraction over two concepts in M3DB called [placements](../operational_guide/placement) and [namespaces](../operational_guide/namespace_configuration). If a placement doesn't exist, it will create one based on the `type` argument, otherwise if the placement already exists, it just creates the specified namespace. For now its enough to just understand that it creates M3DB namespaces (tables), but if you're going to run a clustered M3 setup in production, make sure you familiarize yourself with the links above.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these links need the .md suffixes?


## Namespace Operations

The operations below include sample CURLs, but you can always review the API documentation by navigating to
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/CURL/cURL here and other mentions to cURL.

CreateURL = handler.RoutePrefixV1 + "/database/create"

// CreateHTTPMethod is the HTTP method used with this resource.
// CreateNamespaceURL is the URL for the database namespace create handler.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: double space to single space.


### Modifying a Namespace

There is currently no atomic namespace modification endpoint. Instead, you will need to delete a namespace and then add it back again with the same name, but modified settings. Review the individual namespace settings above to determine whether or not a given setting is safe to modify. For example,it is never safe to modify the blockSize of a namespace.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: space after comma.

src/query/api/v1/handler/database/create.go Show resolved Hide resolved
src/query/api/v1/handler/database/create.go Show resolved Hide resolved
Richard Artoul added 2 commits February 4, 2019 23:11
Copy link
Collaborator

@justinjc justinjc left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-io
Copy link

codecov-io commented Feb 6, 2019

Codecov Report

Merging #1350 into master will increase coverage by <.1%.
The diff coverage is 69.9%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1350     +/-   ##
========================================
+ Coverage    70.7%   70.7%   +<.1%     
========================================
  Files         822     822             
  Lines       70204   70289     +85     
========================================
+ Hits        49653   49724     +71     
- Misses      17322   17329      +7     
- Partials     3229    3236      +7
Flag Coverage Δ
#aggregator 82.3% <ø> (ø) ⬆️
#cluster 85.6% <ø> (ø) ⬆️
#collector 78.4% <ø> (ø) ⬆️
#dbnode 80.8% <ø> (ø) ⬆️
#m3em 73.2% <ø> (ø) ⬆️
#m3ninx 74.2% <ø> (-0.1%) ⬇️
#m3nsch 51.1% <ø> (ø) ⬆️
#metrics 17.8% <ø> (ø) ⬆️
#msg 74.9% <ø> (ø) ⬆️
#query 64.3% <70%> (ø) ⬆️
#x 76% <66.6%> (-0.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 a0cf5b5...d99a0a0. Read the comment docs.

<!-- TODO: link to aggregation documentation (outside of query.md) -->

```json
local:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably have this somewhere, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aggregation is hard to talk about without the Prometheus context, so I added a link in the Prometheus integration section to the Query aggregation docs

docs/how_to/single_node.md Outdated Show resolved Hide resolved
src/query/api/v1/handler/database/create.go Outdated Show resolved Hide resolved
src/query/api/v1/handler/database/create.go Show resolved Hide resolved
src/query/api/v1/handler/database/create.go Show resolved Hide resolved
return nil, true, errCantReplaceLocalPlacementWithClustered
}

// This is fine because we'll just assume they want to keep the same clustered placement
Copy link
Collaborator

Choose a reason for hiding this comment

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

worth logging this so that the user knows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the placement comes back in the API response, so I think its ok

src/query/api/v1/handler/database/create.go Outdated Show resolved Hide resolved
src/x/net/http/convert.go Outdated Show resolved Hide resolved
@richardartoul richardartoul merged commit 9a5b945 into master Feb 6, 2019
@justinjc justinjc deleted the ra/fix-database-create 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.

4 participants